Closed Bug 1184867 Opened 5 years ago Closed 5 years ago

Update WebMContainerParser to be compatible with new MSE

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jya, Assigned: j)

References

Details

Attachments

(3 files, 6 obsolete files)

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();
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.
Assignee: nobody → j
Attachment #8636536 - Flags: review?(jyavenard)
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.
Attachment #8636536 - Attachment is obsolete: true
Attachment #8636536 - Flags: review?(jyavenard)
Attachment #8636679 - Flags: review?(jyavenard)
Depends on: 1165772
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.
Attachment #8636679 - Flags: review?(jyavenard) → review?(kinetik)
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.
(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?
no longer block WebM if media.mediasource.format-reader is enabled
Attachment #8637763 - Flags: review?(jyavenard)
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"
Attachment #8637763 - Flags: review?(jyavenard) → review+
(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.
Blocks: 1187247
Comment on attachment 8637763 [details] [diff] [review]
Bug-1184867-P2.-Enable-WebM-in-MediaSource.patch

move to bug 1187247
Attachment #8637763 - Attachment is obsolete: true
(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
Attachment #8636679 - Attachment is obsolete: true
Attachment #8636679 - Flags: review?(kinetik)
Attachment #8639370 - Flags: review?(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...
Attachment #8639370 - Flags: review?(kinetik) → review+
(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+
Attachment #8639370 - Attachment is obsolete: true
Attachment #8639714 - Flags: review+
Attached file test_MultipleInitSegments.html (obsolete) —
(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)]
Attachment #8639719 - Flags: review?(jyavenard)
(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
(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.
Attachment #8639792 - Flags: review?(jyavenard)
Attachment #8639792 - Flags: review?(jyavenard) → review+
Blocks: 1188341
Comment on attachment 8639719 [details]
test_MultipleInitSegments.html

Move tests to there own bug: Bug 1188341
Attachment #8639719 - Attachment is obsolete: true
Attachment #8639719 - Flags: review?(jyavenard)
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
split integration, carrying r+
Attachment #8639714 - Attachment is obsolete: true
Attachment #8640500 - Flags: review+
split integration, carrying r+
Attachment #8640501 - Flags: review+
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.
sorry had to back this out for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=12300657&repo=mozilla-inbound
Flags: needinfo?(j)
(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.
Flags: needinfo?(j) → needinfo?(jyavenard)
(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
Depends on: 1189588
Flags: needinfo?(jyavenard)
You need to log in before you can comment on or make changes to this bug.