Bug 189782

Created attachment 350185 [details] Patch

Comment on attachment 350185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350185&action=review > Source/WebCore/ChangeLog:7 > + should not overlap. Unfortunately, there are lots of muxes out there muxers > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1696 > + // lastEnqueuedDecodeEndTime or even a bit before that to accomodate muxes with imprecise timing information. muxers > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1698 > + // There are many muxes out there where the frame times are not perfectly contiguous, therefore a tolerance is needed. muxers

(In reply to Xabier Rodríguez Calvar from comment #2) > Comment on attachment 350185 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=350185&action=review > > > Source/WebCore/ChangeLog:7 > > + should not overlap. Unfortunately, there are lots of muxes out there > > muxers > > > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1696 > > + // lastEnqueuedDecodeEndTime or even a bit before that to accomodate muxes with imprecise timing information. > > muxers > > > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1698 > > + // There are many muxes out there where the frame times are not perfectly contiguous, therefore a tolerance is needed. > > muxers Not really: Muxer: Application that muxes audio or video frames. Muxing: Encoding a sequence of frames into a file for later playback. Mux: File generated by a muxer. Muxes: Plural of mux.

Since "mux (noun)" is not a word used very often and could still lead to confusion, I'll change it to "file".

Created attachment 350190 [details] Patch

Comment on attachment 350190 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350190&action=review > Source/WebCore/ChangeLog:17 > + Reviewed by Xabier Rodriguez-Calvar. This should go in line 6

Created attachment 350192 [details] Patch

Comment on attachment 350192 [details] Patch Clearing flags on attachment: 350192 Committed r236258: <https://trac.webkit.org/changeset/236258>

All reviewed patches have been landed. Closing bug.

(In reply to Alicia Boya García from comment #0) > Ideally, container formats should use exact timestamps and frames should not > overlap. Unfortunately, there are lots of muxes out there where this is not > always the case. > > This is particularly a problem in WebM, where timestamps are expressed in a > power of 10 timescale, which forces some rounding. Ugh. > This patch makes SourceBuffer allow frames with a small overlaps (<=1ms) as > those usually found in WebM. 1 ms is chosen because it's the default time > scale of WebM files. > > This bug is reproducible on GStreamer-based WebKit ports using VP9 video on > this URL: https://www.youtube.com/watch?v=XWytUOkaydo at > PTS=0:00:05.367000000. In absence of this patch, that frame is not enqueued > and playback gets stuck. How did this overlap manage to survive the parsing process? Particularly step 1.14.1: "If last decode timestamp for track buffer is unset and presentation timestamp falls within the presentation interval of a coded frame in track buffer, then run the following steps..." The SampleMap makes some assumptions about its constituent samples, namely that they do not overlap. If we are allowing samples in the map to overlap, there may be other subtle bugs in the SourceBuffer code. Rather than patch up this behavior at render time, I'd rather we trim the duration of overlapping samples, either when they're emitted by the parser, or before they're added to the sample map.

(In reply to Jer Noble from comment #10) > How did this overlap manage to survive the parsing process? Particularly > step 1.14.1: "If last decode timestamp for track buffer is unset and > presentation timestamp falls within the presentation interval of a coded > frame in track buffer, then run the following steps..." To answer my own question, we only replace overlapping frames when their start times are within 1 microsecond of each other.

(In reply to Jer Noble from comment #10) > Rather than patch up this behavior at render time, I'd rather we trim the > duration of overlapping samples, either when they're emitted by the parser, > or before they're added to the sample map. Okay, looking more closely at this patch, I think we should just remove the "1ms" fudge factor, and enqueue any frame whose decodeTimestamp > lastEnqueuedDecodeTime (not lastEnqueuedDecodeEndTime).

(In reply to Jer Noble from comment #12) > (In reply to Jer Noble from comment #10) > > Rather than patch up this behavior at render time, I'd rather we trim the > > duration of overlapping samples, either when they're emitted by the parser, > > or before they're added to the sample map. > > Okay, looking more closely at this patch, I think we should just remove the > "1ms" fudge factor, and enqueue any frame whose decodeTimestamp > > lastEnqueuedDecodeTime (not lastEnqueuedDecodeEndTime). It's an option I would not oppose. Durations are somewhat unreliable (especially in WebM), so the least we depend on them, the best.