Enable new MediaSource architecture by default

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Turn on the new MediaSourceDemuxer and all the new MSE architecture goodness.
Enable MediaSourceDemuxer. At this stage, you don't want to enable it :(
Attachment #8615689 - Flags: review?(ajones)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Attachment #8615689 - Flags: review?(ajones) → review+
Depends on: 1173657
Depends on: 1171314
Depends on: 1174582
Depends on: 1174583
Depends on: 1174584
Depends on: 1174586
Depends on: 1174588
Depends on: 1174981
Depends on: 1175037
Depends on: 1175058
Depends on: 1175059
Depends on: 1175395
Depends on: 1176178
Depends on: 1176494
Depends on: 1176918
Depends on: 1176923
Depends on: 1176989
Depends on: 1181894
Depends on: 1182985
Depends on: 1182999
Depends on: 1183573
Depends on: 1183482
Depends on: 1182945
Depends on: 1183519
Posted patch P2. update webref. (obsolete) — Splinter Review
Attachment #8633901 - Flags: review?(karlt)
Blocks: 1066467
Depends on: 1184043
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)
(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)
Blocks: 1184826
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)
Attachment #8615689 - Attachment is obsolete: true
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
Blocks: 1105760
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+
Blocks: 1165772
Depends on: 1186261
https://hg.mozilla.org/mozilla-central/rev/a465aecfd856
https://hg.mozilla.org/mozilla-central/rev/c100ddbbaf68
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
backed out the pref change due to smoke test breakage
https://hg.mozilla.org/mozilla-central/rev/706318c7d595
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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: 4 years ago4 years ago
Resolution: --- → FIXED
Blocks: 1187542
Depends on: 1186965
Depends on: 1180935
Depends on: 1188651
No longer depends on: 1180935
Depends on: 1190258
Depends on: 1191334
Depends on: 1191612
You need to log in before you can comment on or make changes to this bug.