1174577 - TrackBuffersManager not properly handling input with multiple init segment.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

If the input buffer contains more than one init segment. TrackBuffersManager will continue to parse the same one.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: P2. Properly handle multiple init segments.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: P1. Have Init segment range only include the first moov.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8622187 [details] [diff] [review]
P2. Properly handle multiple init segments.

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

Passing review to Alfredo. Thanks for taking on this responsibility Alfredo!

Comment 4

[Alfredo Yang (:alfredo)]

Comment on attachment 8622187 [details] [diff] [review]
P2. Properly handle multiple init segments.

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

::: dom/media/mediasource/TrackBuffersManager.h
@@ +143,5 @@
>    void RecreateParser();
>    nsAutoPtr<ContainerParser> mParser;
>  
>    // Demuxer objects and methods.
> +  nsRefPtr<MediaLargeByteBuffer> mInitData;

RecreateParser() creates a new parser when new init segment receiving in SegmentParserLoop(), and the new init segment can always be retrieved from mParser->InitData(). Is it necessary to add mInitData?

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to Alfredo Yang from comment #4)
> Comment on attachment 8622187 [details] [diff] [review]
> P2. Properly handle multiple init segments.
> 
> Review of attachment 8622187 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/mediasource/TrackBuffersManager.h
> @@ +143,5 @@
> >    void RecreateParser();
> >    nsAutoPtr<ContainerParser> mParser;
> >  
> >    // Demuxer objects and methods.
> > +  nsRefPtr<MediaLargeByteBuffer> mInitData;
> 
> RecreateParser() creates a new parser when new init segment receiving in
> SegmentParserLoop(), and the new init segment can always be retrieved from
> mParser->InitData(). Is it necessary to add mInitData?

Very good observation and the answer is yes.

While the ContainerParser will be re-created, we must force the re-creation of the demuxer as it could have been left in a corrupted state (like with an incomplete media segment). We can't do that in CompleteResetParserState as the initialisation of the demuxer is an async process (plus that would be a lot of duplicated code).
the mCurrentInputBuffer won't be reset until a new init segment is seen, we must force an initialisation of the demuxer. The easiest way to do that is to add an init segment to mInputBuffer which will trigger a change to PARSING_INIT_SEGMENT append state.

Now, we can't directly re-use the MediaByteBuffer returned by the ContainerParser and store it in mInputBuffer as if a new appendBuffer is called with new data, it will be appended to mInputBuffer ; this would corrupt the init segment held by the container parser (as it's shared).

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5a99f92a00
https://hg.mozilla.org/integration/mozilla-inbound/rev/246ecb6b92a0

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a5a99f92a00
https://hg.mozilla.org/mozilla-central/rev/246ecb6b92a0