Closed Bug 1240201 Opened 4 years ago Closed 4 years ago

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


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

45 Branch
Not set





(Reporter: jya, Assigned: jya)


(Blocks 1 open bug)



(3 files)

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.
See Also: → 1237629
Summary: WebM/Vorbis use with MSE will report incorrect buffered range → WebM/Vorbis or WebM/Opus use with MSE may report incorrect buffered range
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.
Attachment #8710027 - Flags: review?(kinetik)
Assignee: nobody → jyavenard
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 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)
Attachment #8710024 - Flags: review?(kinetik) → review+
Attachment #8710027 - Flags: review?(kinetik) → review+
Use a VorbisPacketSampleCounter helper class as recommended.
Carrying r+
Attachment #8710299 - Flags: review+
Flags: needinfo?(jyavenard)
See Also: → 1241057
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:
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).
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.
Closed: 4 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.