Closed
Bug 1171379
Opened 10 years ago
Closed 9 years ago
Enable new MediaSource architecture by default
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: jya, Assigned: jya)
References
Details
Attachments
(2 files, 2 obsolete files)
28.45 KB,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
988 bytes,
patch
|
Details | Diff | Splinter Review |
Turn on the new MediaSourceDemuxer and all the new MSE architecture goodness.
Assignee | ||
Comment 1•10 years ago
|
||
Enable MediaSourceDemuxer. At this stage, you don't want to enable it :(
Attachment #8615689 -
Flags: review?(ajones)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Updated•9 years ago
|
Attachment #8615689 -
Flags: review?(ajones) → review+
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8633901 -
Flags: review?(karlt)
Assignee | ||
Comment 3•9 years ago
|
||
Comment 4•9 years ago
|
||
Comment on attachment 8633901 [details] [diff] [review]
P2. update webref.
>Bug 1171379: P2. update webref. r=karlt
I assume the changes here are due to switching the
media.mediasource.format-reader pref?
Please include a reference to this or MediaSourceDemuxer in the comment.
But why are so many tests now annotated as failing?
mediasource-append-buffer.html is now enabled, so failures there are not
concerning (though I would have enabled the test in a separate patch to all
the other result changes required for the pref change).
But don't most of the rest of the changes here indicate that the tests
are catching regressions?
> [mediasource-buffered.html]
> type: testharness
> prefs: [media.mediasource.enabled:true, media.mediasource.whitelist:false]
>+ disabled:
>+ if (os == "win") and (version == "5.1.2600"): https://bugzilla.mozilla.org/show_bug.cgi?id=1182945
Oh, so switching media.mediasource.format-reader also disables webm, even with
user_pref("media.mediasource.webm.enabled", true) in
testing/profiles/prefs_general.js.
It would have been helpful to explain that in the commit message.
With all the changes here, it is very hard to see what effect, if any, the
media.mediasource.format-reader switch has on mp4 results.
An easy way to see that is to remove the media.mediasource.webm.enabled
line from testing/profiles/prefs_general.js and run with
media.mediasource.format-reader false.
Would such a run with this patch generate the results expected here?
There doesn't seem to be much point in having media.mediasource.webm.enabled
explicitly set if there is no attempt to support it.
The appropriate approach here would seem to be to first disable webm (by removing the line from prefs_general.js) and update the expectation data in line with that. (If there are infrequent intermittents that will be resolved through the media.mediasource.format-reader switch, then there is no need to disable tests just for this.) Then the media.mediasource.format-reader pref can be toggled, presumably without requiring annotation of many new test failures.
Flags: needinfo?(jyavenard)
Attachment #8633901 -
Flags: review?(karlt)
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Karl Tomlinson (ni?:karlt) from comment #4)
> Comment on attachment 8633901 [details] [diff] [review]
> P2. update webref.
>
> >Bug 1171379: P2. update webref. r=karlt
>
> I assume the changes here are due to switching the
> media.mediasource.format-reader pref?
> Please include a reference to this or MediaSourceDemuxer in the comment.
>
> But why are so many tests now annotated as failing?
Only the webm ones.
The new MSE doesn't support webm at this stage.
Most of the MP4 tests now passes with the exception of tests checking that the buffered ranges start at 0. This is a bug in the tests themselves that are using samples not starting at 0 (they all start at 0.095) I've raised a bug to them for that.
>
> mediasource-append-buffer.html is now enabled, so failures there are not
> concerning (though I would have enabled the test in a separate patch to all
> the other result changes required for the pref change).
>
> But don't most of the rest of the changes here indicate that the tests
> are catching regressions?
>
> > [mediasource-buffered.html]
> > type: testharness
> > prefs: [media.mediasource.enabled:true, media.mediasource.whitelist:false]
> >+ disabled:
> >+ if (os == "win") and (version == "5.1.2600"): https://bugzilla.mozilla.org/show_bug.cgi?id=1182945
>
> Oh, so switching media.mediasource.format-reader also disables webm, even
> with
> user_pref("media.mediasource.webm.enabled", true) in
> testing/profiles/prefs_general.js.
>
> It would have been helpful to explain that in the commit message.
Can add that..
Sorry, this was very obvious to me as most of the work related to enabling the new MSE is disabling all webm tests.
>
> With all the changes here, it is very hard to see what effect, if any, the
> media.mediasource.format-reader switch has on mp4 results.
I'm not sure how this could be done.
90% of this commit is auto-generated by web-platform-tests-update
The mac results were taken as the default, and overwrote all other tests.
I then confirmed that Win7 and later all behave in an identical fashion.
I used a linux VM to generate the results and manually modifed so the results would be generic to all the linux.
I then manually crafted the XP ones.
>
> An easy way to see that is to remove the media.mediasource.webm.enabled
> line from testing/profiles/prefs_general.js and run with
> media.mediasource.format-reader false.
> Would such a run with this patch generate the results expected here?
Unfortunately no.
The new MSE is much more spec compliant. It passes much more tests than before.
We do fail differently for some tests, in particular mediasource-buffered as I mentioned above.
>
> There doesn't seem to be much point in having media.mediasource.webm.enabled
> explicitly set if there is no attempt to support it.
But it is off already by default no?
pref("media.mediasource.webm.enabled", false);
Edit: Ohhh I see what you mean with your comment later:
testing/profiles/prefs_general.js
I see in MediaSource.cpp IsTypeSupported, it returns NS_ERROR_DOM_INVALID_STATE_ERR with media.mediasource.webm.enabled while it returns NS_ERROR_DOM_NOT_SUPPORTED_ERR (as per spec) if the new mse is active.
Having said that, regardless of what we report, we have this bug 1165772, I wonder if there's the same problem with webm.
>
> The appropriate approach here would seem to be to first disable webm (by
> removing the line from prefs_general.js) and update the expectation data in
> line with that. (If there are infrequent intermittents that will be
> resolved through the media.mediasource.format-reader switch, then there is
> no need to disable tests just for this.) Then the
> media.mediasource.format-reader pref can be toggled, presumably without
> requiring annotation of many new test failures.
I'm not sure what there is to gain in doing it in two steps.
Who is ever going to bother looking at a diff when it comes to meta results ?
Especially automatically generated config.
It takes over a day to test all platforms via try, going back and forth several times...
Doing it all over again is less than appealing for very little gain I feel :(
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 6•9 years ago
|
||
Failure on Linux and Windows XP are a consequence of no MSE available at all.
Failure on Mac and Windows >= 7 with two exceptions are due to the tests using a malformed video.
The bug was reported upstream: https://github.com/w3c/web-platform-tests/issues/1939
Attachment #8635096 -
Flags: review?(karlt)
Assignee | ||
Comment 7•9 years ago
|
||
Rebasing...
Assignee | ||
Updated•9 years ago
|
Attachment #8615689 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8633901 [details] [diff] [review]
P2. update webref.
Removal of webm support was done in bug 1182945
Attachment #8633901 -
Attachment is obsolete: true
Comment 9•9 years ago
|
||
Comment on attachment 8635096 [details] [diff] [review]
P2. Update web-platform-tests expected data.
This provides a lot of confidence in the new architecture, thanks.
>+++ b/testing/web-platform/meta/media-source/interfaces.html.ini
>@@ -1,64 +1,4 @@
> [interfaces.html]
> type: testharness
> prefs: [media.mediasource.enabled:true, media.mediasource.whitelist:false]
> expected: ERROR
>- [AudioTrack interface: attribute kind]
>- expected: FAIL
>-
>- [AudioTrack interface: attribute language]
>- expected: FAIL
>-
>- [AudioTrack interface: attribute sourceBuffer]
>- expected: FAIL
>-
>- [VideoTrack interface: attribute kind]
>- expected: FAIL
>-
>- [VideoTrack interface: attribute language]
>- expected: FAIL
>-
>- [VideoTrack interface: attribute sourceBuffer]
>- expected: FAIL
>-
>- [TextTrack interface: attribute kind]
>- expected: FAIL
>-
>- [TextTrack interface: attribute language]
>- expected: FAIL
>-
>- [TextTrack interface: attribute sourceBuffer]
>- expected: FAIL
>-
>- [SourceBuffer interface: attribute audioTracks]
>- expected: FAIL
>-
>- [SourceBuffer interface: attribute videoTracks]
>- expected: FAIL
>-
>- [SourceBuffer interface: attribute textTracks]
>- expected: FAIL
>-
>- [SourceBuffer interface: operation appendStream(Stream,unsigned long long)]
>- expected: FAIL
>-
>- [SourceBuffer interface: sourceBuffer must inherit property "audioTracks" with the proper type (4)]
>- expected: FAIL
>-
>- [SourceBuffer interface: sourceBuffer must inherit property "videoTracks" with the proper type (5)]
>- expected: FAIL
>-
>- [SourceBuffer interface: sourceBuffer must inherit property "textTracks" with the proper type (6)]
>- expected: FAIL
>-
>- [SourceBuffer interface: sourceBuffer must inherit property "appendStream" with the proper type (11)]
>- expected: FAIL
>-
>- [SourceBuffer interface: calling appendStream(Stream,unsigned long long) on sourceBuffer with too few arguments must throw TypeError]
>- expected: FAIL
>-
>- [VideoPlaybackQuality interface: attribute totalFrameDelay]
>- expected: FAIL
>-
>- [VideoPlaybackQuality interface: video.getVideoPlaybackQuality() must inherit property "totalFrameDelay" with the proper type (4)]
>- expected: FAIL
These changes are not a result of changes in this bug, but are possible
because the ERROR that was introduced in bug 1182945 means that the subtests
don't run. They should be moved to the patch in that bug.
> [mediasource-config-change-mp4-av-audio-bitrate.html]
> type: testharness
> prefs: [media.mediasource.enabled:true, media.mediasource.whitelist:false]
>+ disabled: https://bugzilla.mozilla.org/show_bug.cgi?id=1130973
Please update bug 1130973 with the details of the current errors or timeouts
and note that the bug now affects all platforms.
Attachment #8635096 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a465aecfd856
https://hg.mozilla.org/mozilla-central/rev/c100ddbbaf68
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
\o/
backed out the pref change due to smoke test breakage
https://hg.mozilla.org/mozilla-central/rev/706318c7d595
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 15•9 years ago
|
||
This is totally silly.
The smoke test failing doesn't even use MSE.
Re-resolving for the un-backout from comment 16.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Depends on: 1190019
Updated•9 years ago
|
status-firefox41:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•