1184867 - Update WebMContainerParser to be compatible with new MSE

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

WebMContainerParser needs to be updated to work with the new MSE.

The functions required are:

  bool HasCompleteInitData();
  // Returns the byte range of the first complete init segment, or an empty
  // range if not complete.
  MediaByteRange InitSegmentRange();
  // Returns the byte range of the first complete media segment header,
  // or an empty range if not complete.
  MediaByteRange MediaHeaderRange();
  // Returns the byte range of the first complete media segment or an empty
  // range if not complete.
  MediaByteRange MediaSegmentRange();

Comment 1

[Jan Gerber]

Attachment: Bug-1184867-Update-WebMContainerParser.patch

I still see calls to RecreateParser followed by SegmentParserLoop looking at data thats not a media segment. Was not able to track down where this goes wrong so far.

Comment 2

[Jan Gerber]

Attachment: Bug-1184867-Update-WebMContainerParser.patch

found it: since the return values is no longer checked in new MSE code. only return complete ranges if buffer does not end with a partial media segment.

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8636679 [details] [diff] [review]
Bug-1184867-Update-WebMContainerParser.patch

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

I don't know enoug about webm.. Matthew would be much better to review this.

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8636679 [details] [diff] [review]
Bug-1184867-Update-WebMContainerParser.patch

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

From a quick look: if you have multiple media segment in there, it seems to me that you media byte range would cover all of them rather than the first one only...

And what would happen if the data added was made of:
Init segment | media segment | init segment | media segment

The 2nd init segment would be missed. 

While an unlikely scenario with YouTube (and they seem to be the only ones using webm), it is a scenario we have to handle.
The YouTube conformance suite had a test doing that with MP4.

Comment 5

[Jan Gerber]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> Comment on attachment 8636679 [details] [diff] [review]
> Bug-1184867-Update-WebMContainerParser.patch
> 
> Review of attachment 8636679 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> From a quick look: if you have multiple media segment in there, it seems to
> me that you media byte range would cover all of them rather than the first
> one only...
> 
> And what would happen if the data added was made of:
> Init segment | media segment | init segment | media segment
> 
> The 2nd init segment would be missed. 

You mean if multiple init segments are passed in one sb.appendBuffer call?
Is that supported by the spec?

Comment 6

[Jan Gerber]

Attachment: Bug-1184867-P2.-Enable-WebM-in-MediaSource.patch

no longer block WebM if media.mediasource.format-reader is enabled

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8637763 [details] [diff] [review]
Bug-1184867-P2.-Enable-WebM-in-MediaSource.patch

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

I had intended this patch to be in its own bug however "Enable WebM with new MSE"

Comment 8

[Jean-Yves Avenard [:jya]]

(In reply to Jan Gerber from comment #5)

> You mean if multiple init segments are passed in one sb.appendBuffer call?
> Is that supported by the spec?

There is nothing that says it can't be

If you look at the Segment Parser Loop, they specifically fragment every segment on its own, and process just one segment (init or media) at a time.

While I'm guessing it's unlikely to be done, it could be.

Comment 9

[Jan Gerber]

Comment on attachment 8637763 [details] [diff] [review]
Bug-1184867-P2.-Enable-WebM-in-MediaSource.patch

move to bug 1187247

Comment 10

[Jan Gerber]

Attachment: Bug-1184867-Update-WebMContainerParser.patch

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> Comment on attachment 8636679 [details] [diff] [review]
> Bug-1184867-Update-WebMContainerParser.patch
> 
> From a quick look: if you have multiple media segment in there, it seems to
> me that you media byte range would cover all of them rather than the first
> one only...

updated patch to only return the media range of the first media segment.

> And what would happen if the data added was made of:
> Init segment | media segment | init segment | media segment
> 
> The 2nd init segment would be missed. 

Where is this case handled for mp4?
My understanding would be that since now only the first media range is returned,
TrackBuffersManager detects the new init segment in WAITING_FOR_SEGMENT

Comment 11

[Matthew Gregan [:kinetik]]

Comment on attachment 8639370 [details] [diff] [review]
Bug-1184867-Update-WebMContainerParser.patch

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

LGTM.

I think we need a to add a mochitest for WebM and MP4 covering the multiple init case.

::: dom/media/platforms/agnostic/VPXDecoder.cpp
@@ +135,4 @@
>                                                aSample->mDuration,
>                                                b,
>                                                aSample->mKeyframe,
> +                                              aSample->mTimecode,

This change seems out of place for this patch...

Comment 12

[Jan Gerber]

Attachment: Bug-1184867-Update-WebMContainerParser.patch

(In reply to Matthew Gregan [:kinetik] from comment #11)
> ::: dom/media/platforms/agnostic/VPXDecoder.cpp
> @@ +135,4 @@
> >                                                aSample->mDuration,
> >                                                b,
> >                                                aSample->mKeyframe,
> > +                                              aSample->mTimecode,
> 
> This change seems out of place for this patch...

ok, removed. might be needed at a later point but ffmpeg backend also always sends -1 here.

Carrying r+

Comment 13

[Jan Gerber]

Attachment: test_MultipleInitSegments.html

(In reply to Matthew Gregan [:kinetik] from comment #11)
> I think we need a to add a mochitest for WebM and MP4 covering the multiple
> init case.

adding test for webm. the samples get parsed ok but somehow loadedmetadata is not fired v.duration is already correct. jya do you have an idea where this could be stuck?

TrackBuffersManager::OnVideoDemuxCompleted: 30 video samples demuxed
TrackBuffersManager::InsertFrames: Processing 30 video/webm; codecs=vp8 frames(start:0 end:1001000)
TrackBuffersManager::UpdateBufferedRanges: after video ranges=[(0.000000, 1.001000)]
TrackBuffersManager::OnVideoDemuxCompleted: 12 video samples demuxed
TrackBuffersManager::InsertFrames: Processing 12 video/webm; codecs=vp8 frames(start:800000 end:1201000)
TrackBuffersManager::RemoveFrames: Removing frames from:24 (frames:6) ([0.800000, 1.001000))
TrackBuffersManager::UpdateBufferedRanges: after video ranges=[(0.000000, 1.201000)]
SourceBuffer::GetBuffered: ranges=[(0.000000, 1.201000)]

Comment 14

[Jean-Yves Avenard [:jya]]

(In reply to Jan Gerber from comment #12)

> ok, removed. might be needed at a later point but ffmpeg backend also always
> sends -1 here.
> 
> Carrying r+

Getting proper dts is essential with the new MSE. And the time base for them must be the same as the pts so we can properly insert the frames in the vector of frames and that they keep monotonic timestamps.

For playback, it doesn't matter. That's why ffmpeg can always set -1

Comment 15

[Jan Gerber]

Attachment: Bug-1184867-VPXDecoder-pass-DTS-to-VideoData-Create.patch

(In reply to Jean-Yves Avenard [:jya] from comment #14)
> (In reply to Jan Gerber from comment #12)
> 
> > ok, removed. might be needed at a later point but ffmpeg backend also always
> > sends -1 here.
> > 
> > Carrying r+
> 
> Getting proper dts is essential with the new MSE. And the time base for them
> must be the same as the pts so we can properly insert the frames in the
> vector of frames and that they keep monotonic timestamps.
> 
> For playback, it doesn't matter. That's why ffmpeg can always set -1

ok adding it as an extra patch.

Comment 16

[Jan Gerber]

Comment on attachment 8639719 [details]
test_MultipleInitSegments.html

Move tests to there own bug: Bug 1188341

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8639714 [details] [diff] [review]
Bug-1184867-Update-WebMContainerParser.patch

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

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +15,5 @@
>  
> +#ifdef MOZ_WEBM
> +#include "WebMDemuxer.h"
> +#endif
> +

please split the integration into a 3rd patch

Comment 18

[Jan Gerber]

Attachment: Bug-1184867-Update-WebMContainerParser.patch

split integration, carrying r+

Comment 19

[Jan Gerber]

Attachment: Bug-1184867-Use-WebMDemuxer-in-TrackBuffersManager.patch

split integration, carrying r+

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8639792 [details] [diff] [review]
Bug-1184867-VPXDecoder-pass-DTS-to-VideoData-Create.patch

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

::: dom/media/platforms/agnostic/VPXDecoder.cpp
@@ -135,4 @@
>                                                aSample->mDuration,
>                                                b,
>                                                aSample->mKeyframe,
> -                                              -1,

actually, this is the decoding part, dts aren't that important. My earlier comments were about the demuxer only.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a093b996bafa
https://hg.mozilla.org/integration/mozilla-inbound/rev/4624de03e2e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b17cd283110

Comment 22

[Carsten Book [:Tomcat]]

sorry had to back this out for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=12300657&repo=mozilla-inbound

Comment 23

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1bf2e8f78aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea0bf76ee44e
https://hg.mozilla.org/integration/mozilla-inbound/rev/82b165aa3192

Comment 24

[Jan Gerber]

(In reply to Carsten Book [:Tomcat] from comment #22)
> sorry had to back this out for crashes like
> https://treeherder.mozilla.org/logviewer.html#?job_id=12300657&repo=mozilla-
> inbound

could this be related to other changes? media.mediasource.webm.enabled is not enabled
right now and so this code would not be active.

Comment 25

[Jan Gerber]

(In reply to Jan Gerber from comment #24)
> (In reply to Carsten Book [:Tomcat] from comment #22)
> > sorry had to back this out for crashes like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=12300657&repo=mozilla-
> > inbound

mediasource-buffered.html on windows 8 64bit will run those tests with mp4
so this crash is caused by h264 or aac. why was it only on the pgo build?

I found one possible cause for the crash:

It did not crash for the build related to the commit[1]:

04:37:34     INFO - TEST-START | /media-source/mediasource-buffered.html
04:37:34     INFO - PROCESS | 896 | E/MPEG4Extractor(  896): No width or height, assuming worst case 1080p
04:37:38     INFO - TEST-FAIL | /media-source/mediasource-buffered.html | Demuxed content with different lengths - assert_equals: mediaSource.activeSourceBuffers[1] expected "{ [0.000, 2.000) }" but got "{ [0.067, 2.067) }"

While only a later commit[2] crashed[3]:

05:39:15     INFO - TEST-START | /media-source/mediasource-buffered.html
05:39:15     INFO - PROCESS | 1416 | E/MPEG4Extractor( 1416): No width or height, assuming worst case 1080p
05:39:26     INFO - PROCESS-CRASH | /media-source/mediasource-buffered.html | application crashed [@ mozilla::MediaSourceTrackDemuxer::NotifyTimeRangesChanged()]

The crash commit also reverted something in TrackBuffersManager, that looks like it could be the cause of this[4] (part of  Bug 1189138). 
Looking at the other patches in Bug 1189138 I think the cause of the crash is 
 Bug 1189138: [MSE] P4. Tell the mediasource demuxer of modified range as early as possible. r=gerald [5]

1) https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win64-pgo/1438240026/mozilla-inbound_win8_64_test_pgo-web-platform-tests-3-bm112-tests1-windows-build329.txt.gz
2) https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bce7955eaa13
3)  https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win64-pgo/1438245016/mozilla-inbound_win8_64_test_pgo-web-platform-tests-3-bm110-tests1-windows-build244.txt.gz
4) https://hg.mozilla.org/integration/mozilla-inbound/rev/bce7955eaa13
5) https://hg.mozilla.org/integration/mozilla-inbound/rev/42a673cd36ff

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/989feeae39f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/10f31a12afb6
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5aa4bab42ba

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/989feeae39f0
https://hg.mozilla.org/mozilla-central/rev/10f31a12afb6
https://hg.mozilla.org/mozilla-central/rev/e5aa4bab42ba