Update WebMContainerParser to be compatible with new MSE

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: j)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments, 6 obsolete attachments)

Reporter

Description

4 years ago
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();
Assignee

Comment 1

4 years ago
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)
Assignee

Comment 2

4 years ago
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)
Assignee

Updated

4 years ago
Depends on: 1165772
Reporter

Comment 3

4 years ago
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)
Reporter

Comment 4

4 years ago
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.
Assignee

Comment 5

4 years ago
(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?
Assignee

Comment 6

4 years ago
no longer block WebM if media.mediasource.format-reader is enabled
Attachment #8637763 - Flags: review?(jyavenard)
Reporter

Comment 7

4 years ago
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+
Reporter

Comment 8

4 years ago
(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.
Assignee

Updated

4 years ago
Blocks: 1187247
Assignee

Comment 9

4 years ago
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
Assignee

Comment 10

4 years ago
(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+
Assignee

Comment 12

4 years ago
(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+
Assignee

Comment 13

4 years ago
Posted 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)
Reporter

Comment 14

4 years ago
(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
Assignee

Comment 15

4 years ago
(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)
Reporter

Updated

4 years ago
Attachment #8639792 - Flags: review?(jyavenard) → review+
Assignee

Updated

4 years ago
Blocks: 1188341
Assignee

Comment 16

4 years ago
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)
Reporter

Comment 17

4 years ago
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
Assignee

Comment 18

4 years ago
split integration, carrying r+
Attachment #8639714 - Attachment is obsolete: true
Attachment #8640500 - Flags: review+
Assignee

Comment 19

4 years ago
split integration, carrying r+
Attachment #8640501 - Flags: review+
Reporter

Comment 20

4 years ago
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)
Assignee

Comment 24

4 years ago
(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)
Assignee

Comment 25

4 years ago
(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
Reporter

Updated

4 years ago
Depends on: 1189588
Reporter

Updated

4 years ago
Flags: needinfo?(jyavenard)
You need to log in before you can comment on or make changes to this bug.