Bug 189782 – [MSE] Use some tolerance when deciding whether a frame should be appended to the decode queue

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alicia Boya García]

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.

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.

[Alicia Boya García]

Created attachment 350185 [details]
Patch

[Xabier Rodríguez Calvar]

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

[Alicia Boya García]

(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.

[Alicia Boya García]

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

[Alicia Boya García]

Created attachment 350190 [details]
Patch

[Xabier Rodríguez Calvar]

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

[Alicia Boya García]

Created attachment 350192 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 350192 [details]
Patch

Clearing flags on attachment: 350192

Committed r236258: <https://trac.webkit.org/changeset/236258>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

(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.

[Jer Noble]

(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.

[Jer Noble]

(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).

[Alicia Boya García]

(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.