bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Update WebMContainerParser to be compatible with new MSE

RESOLVED FIXED in Firefox 42

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jya, Assigned: Jan Gerber)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

3 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

3 years ago
Created attachment 8636536 [details] [diff] [review]
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.
Assignee: nobody → j
Attachment #8636536 - Flags: review?(jyavenard)
(Assignee)

Comment 2

3 years ago
Created attachment 8636679 [details] [diff] [review]
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.
Attachment #8636536 - Attachment is obsolete: true
Attachment #8636536 - Flags: review?(jyavenard)
Attachment #8636679 - Flags: review?(jyavenard)
(Assignee)

Updated

3 years ago
Depends on: 1165772
(Reporter)

Comment 3

3 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

3 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

3 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

3 years ago
Created attachment 8637763 [details] [diff] [review]
Bug-1184867-P2.-Enable-WebM-in-MediaSource.patch

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

Comment 7

3 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

3 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

3 years ago
Blocks: 1187247
(Assignee)

Comment 9

3 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

3 years ago
Created attachment 8639370 [details] [diff] [review]
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
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

3 years ago
Created attachment 8639714 [details] [diff] [review]
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+
Attachment #8639370 - Attachment is obsolete: true
Attachment #8639714 - Flags: review+
(Assignee)

Comment 13

3 years ago
Created attachment 8639719 [details]
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)]
Attachment #8639719 - Flags: review?(jyavenard)
(Reporter)

Comment 14

3 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

3 years ago
Created attachment 8639792 [details] [diff] [review]
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.
Attachment #8639792 - Flags: review?(jyavenard)
(Reporter)

Updated

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

Updated

3 years ago
Blocks: 1188341
(Assignee)

Comment 16

3 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

3 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

3 years ago
Created attachment 8640500 [details] [diff] [review]
Bug-1184867-Update-WebMContainerParser.patch

split integration, carrying r+
Attachment #8639714 - Attachment is obsolete: true
Attachment #8640500 - Flags: review+
(Assignee)

Comment 19

3 years ago
Created attachment 8640501 [details] [diff] [review]
Bug-1184867-Use-WebMDemuxer-in-TrackBuffersManager.patch

split integration, carrying r+
Attachment #8640501 - Flags: review+
(Reporter)

Comment 20

3 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

3 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

3 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

3 years ago
Depends on: 1189588
(Reporter)

Updated

3 years ago
Flags: needinfo?(jyavenard)
https://hg.mozilla.org/mozilla-central/rev/989feeae39f0
https://hg.mozilla.org/mozilla-central/rev/10f31a12afb6
https://hg.mozilla.org/mozilla-central/rev/e5aa4bab42ba
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.