Closed Bug 1174577 Opened 5 years ago Closed 5 years ago

TrackBuffersManager not properly handling input with multiple init segment.

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(2 files)

If the input buffer contains more than one init segment. TrackBuffersManager will continue to parse the same one.
Assignee: nobody → jyavenard
Attachment #8622188 - Flags: review?(ajones) → review+
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!
Attachment #8622187 - Flags: review?(cpearce) → review?(ayang)
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?
Attachment #8622187 - Flags: review?(ayang) → review+
(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).
https://hg.mozilla.org/mozilla-central/rev/6a5a99f92a00
https://hg.mozilla.org/mozilla-central/rev/246ecb6b92a0
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Blocks: 1188341
You need to log in before you can comment on or make changes to this bug.