1240201 - WebM/Vorbis or WebM/Opus use with MSE may report incorrect buffered range

Status: RESOLVED INVALID

(Core :: Audio/Video: Playback, defect)

Description

[Jean-Yves Avenard [:jya]]

This is due to the problem exposed in bug 1239223.

With WebM container, we won't know the exact time of vorbis samples and their duration until we decode them.
Coming out of the WebMDemuxer are samples that will all appear to have the same start time.

This will result in a MSE Vorbis SourceBuffer to have an invalid duration and incorrect buffered range reported.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: [vorbis] P1. Properly determine sample duration and time in webm demuxer.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: [opus] P2. Properly determine sample duration and time in webm demuxer.

Times coming out of the demuxer aren't matching those of the decoder 100%, as the timestamp coming out of the decoder are shifted by the Opus preskip value. This cause a discrepency of 13.5ms between the two times.
However, seeing that our MSE allows for 150ms gaps between audio and video ; it will do just fine.

Comment 3

[Jean-Yves Avenard [:jya]]

note that those two patches cause bug 1239223 and bug 1241062 to become unnecessary: The workaround there only occurs if successive frames frame given to the decoder had the same start time (in which case we added to the start time the duration of the last frame decoded). This no longer happens: every frames passed to the decoder have a discrete start time (except the first vorbis packet which has a duration of 0),

Comment 4

[Matthew Gregan [:kinetik]]

Comment on attachment 8710024 [details] [diff] [review]
[vorbis] P1. Properly determine sample duration and time in webm demuxer.

Review of attachment 8710024 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/webm/WebMDemuxer.cpp
@@ +39,5 @@
>  LazyLogModule gNesteggLog("Nestegg");
>  
> +
> +// Vorbis initialization
> +static ogg_packet InitVorbisPacket(const unsigned char* aData, size_t aLength,

I don't think we want to duplicate this code, just export it from the Vorbis decoder.

::: dom/media/webm/WebMDemuxer.h
@@ +210,5 @@
> +  // Vorbis decoder state
> +  vorbis_info mVorbisInfo;
> +  vorbis_comment mVorbisComment;
> +  int64_t mVorbisPacketCount;
> +  Maybe<long> mVorbisLastBlockSize;

Is it possible to move this state and the new code into a helper class, so that the changes to the demuxer are as simple as for the Opus case?  i.e. one call to fetch the duration of a packet (and for Vorbis another to initialize at setup time)

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: [vorbis] P1. Properly determine sample duration and time in webm demuxer.

Use a VorbisPacketSampleCounter helper class as recommended.
Carrying r+

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69b695ad6d43
https://hg.mozilla.org/integration/mozilla-inbound/rev/b06a049cafb7

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8381cc4aa935 for wpt bustage

https://treeherder.mozilla.org/logviewer.html#?job_id=20219844&repo=mozilla-inbound

Comment 8

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/cea2883034cb

Comment 9

[Jean-Yves Avenard [:jya]]

I posted this in bug 1241057, but it's probably better here:

the time between two webm blocks do not match the sum of the samples in those blocks.
So this cause the buffered range reported to no be continuous:
example:
expected "{ [0.000, 6.050) }" but got "{ [0.013, 0.384) [0.408, 0.779) [0.803, 2.381) [2.405, 3.565) [3.589, 3.960) [3.984, 4.378) [4.402, 5.562) [5.586, 5.957) [5.981, 6.050) }

0.013 because I went by the advice of the vorbis author and considered that the first vorbis block wouldn't produce any audio until the second was seen, and from then the number of samples returned by the decoder would be previous_block/4 + next_block/4

So i will have to handle things differently. For a start to prevent the problem mentioned above where we could have a rounding error between the sum of the duration samples and the time found in the container, I will make it so that the duration of the last sample in the block is always aligned to end where the new block start.

I will also not attempt to determine the duration of a sample if there's only one in a the webm block (as in effect we only care that two samples don't have the same start time and duration).

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0a4a2965692
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8eb98e2e496

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b898185c57fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7062f5c8df3

Comment 12

[Jean-Yves Avenard [:jya]]

I've been rethinking the problem and I think that for MSE, you're better off having multiple frames with the same start time and long duration than splitting them up with possibly wrong duration.

that way for eviction, this group of frames will always be removed all together and can't be split up.

When seeking it will also guarantee that you always seek to the first frame of that block and not in the middle of it which could lead to wrong A/V sync.

Seeing that this is what chrome appears doing (as they test it in their webref) for those value. We're fine the way things are working with bug 1241062 in.