Bug 147252 – [MSE] Incorrect sample timestamps when using "sequence" mode

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Sajid Anwar]

This is my first WebKit bug and (hopefully) future patch so pardon any mistakes in the contribution process!

There are two problems that I've seen in the SourceBuffer implementation when it is in sequence mode:

1. In the coded frame processing algorithm, step 1.3, the condition should be checking if the source buffer's mode is "sequence" and if the group start timestamp is valid. The condition currently looks like this:
    
    if (m_mode == sequenceKeyword())

2. According to the specification, "sequence" mode indicates that "[m]edia segments will be treated as adjacent in time independent of the timestamps in the media segment" [1]. In the current implementation of the coded frame processing algorithm, the timestamp offset (when it is non-zero) is being added to each sample's presentation timestamp and decode timestamp, regardless of the append mode. For the audio MIME types "audio/mpeg" and "audio/aac", the specification says that the generate timestamps flag should be set [2], and therefore the mode will be "sequence". I've found that in some cases (such as in the URL provided in this report), the samples of the audio already have the correct presentation timestamps. Consequently, when the generated timestamp offsets are being added to each sample, the effect is that each sample's presentation timestamp is doubled and the audio plays incorrectly, and sometimes not at all. For "sequence" buffers, it seems that the samples' timestamps should be *set to* the generated timestamps, not offset by them. I believe that falls in line more accurately with the specification.

I have a patch to address these that I will attach shortly.

[1]: https://w3c.github.io/media-source/#idl-def-AppendMode.sequence
[2]: https://w3c.github.io/media-source/byte-stream-format-registry.html

[Jer Noble]

Looking forward to it!

[Sajid Anwar]

Created attachment 257441 [details]
Patch

[Sajid Anwar]

Patch is attached! Took so long to complete because I quickly learned that changing an IDL file causes a lot of files to be recompiled.

Thanks for looking at this, and of course, please let me know if there are any changes to be made.

[Jer Noble]

Comment on attachment 257441 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257441&action=review

This patch looks really good! One quibble:

> LayoutTests/media/audio-test.js:1
> +

There's no need to copy video-test.js -> audio-test.js; it's used to test <audio> elements (and <video> elements with audio-only media) as well. If there's any functionality you need which is missing from video-test.js, I'd rather have you add it to video-test.js.

So I'm r-'ing this for now, but this patch can definitely be r+'d with that change.

[Jer Noble]

BTW, I'm really happy to see the changes to MediaSample / MediaSampleAVFObjC.  We're going to need to expand on those to fix some other outstanding bugs in the way our MSE implementation treats audio samples.

[Sajid Anwar]

Good call re: the audio-test.js, I'll have to make the change and the new patch later tonight.

My motive with these changes is actually to get Google Play Music working with MSE under WebKit/Safari, especially considering the recent Flash Player vulnerabilities. With this patch (and a little user-agent trickery), GPM seems like it may actually mostly work with WebKit, so I'm optimistic! 

(I say "mostly" because songs do stream, but there occasionally seem to be issues with seeking or endOfStream(), though in any case I'll have to look into that further)

[Sajid Anwar]

Created attachment 257498 [details]
Patch

[Sajid Anwar]

Created attachment 257501 [details]
Patch

[Sajid Anwar]

My mistake, previous patch before the current one had merge conflicts with the ChangeLog files since I had forgotten to run update-webkit.

[Jer Noble]

Comment on attachment 257501 [details]
Patch

r=me.  Set cq? if you need to me to mark it for committing.

[Sajid Anwar]

Thanks! Appreciate the quick turnaround.

[WebKit Commit Bot]

Comment on attachment 257501 [details]
Patch

Clearing flags on attachment: 257501

Committed r187377: <http://trac.webkit.org/changeset/187377>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.