Closed Bug 1182946 Opened 4 years ago Closed 4 years ago

Re-enable webm mediasource tests.

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jya, Assigned: j)

References

Details

Attachments

(2 files, 11 obsolete files)

36.79 KB, patch
Details | Diff | Splinter Review
3.26 KB, patch
jya
: review+
Details | Diff | Splinter Review
Once the new webm demuxer and VPx decoder lands, we'll be able to re-enable those tests.
Depends on: 1183482
Attached patch P2. update webref. r=karlt (obsolete) — Splinter Review
Attached patch P2. update webref. r=karlt (obsolete) — Splinter Review
Attachment #8633311 - Attachment is obsolete: true
Assignee: nobody → jyavenard
Depends on: 1183519
Attached patch P2. update webref. r=karlt (obsolete) — Splinter Review
Attachment #8633332 - Attachment is obsolete: true
Depends on: 1183573
Comment on attachment 8633241 [details] [diff] [review]
P1. Enable new MSE based on MediaFormatReader architecture.

wrong bug#
Attachment #8633241 - Attachment is obsolete: true
Attachment #8633241 - Flags: review?(ajones)
Attachment #8633342 - Attachment is obsolete: true
No longer depends on: 1183573
No longer depends on: 1183482
No longer depends on: 1183519
Depends on: 1184867
Depends on: 1091774
I believe 1091774 will be improved by 1173657.
I also don't think it applies with the new MSE as we extract all frames at once.
Blocks: 1190379
- don't enable test_SetModeThrows.html since it no longer throws,
  is this test obsolete with the new MSE?


- test_BufferingWait.html currently fails since waiting is triggered at 0.66 instead of 0.7

Buffered ranges are up to 0.801000

848652032[7f6a26986380]: TrackBuffersManager(7f6a269be000:video/webm)::UpdateBufferedRanges: after video ranges=[(0.000000, 0.801000)]

And it looks like more frames are decoded:

TrackBuffersManager(7f6a269be000:video/webm)::ProcessFrames: Processing video/webm; codecs=vp8 frame(pts:767000 end:801000, dts:767000, duration:34000, kf:0)

Decoder=7f6a2682b8b0 playing video frame 733000 (id=12) (queued=21, state-machine=10, decoder-queued=11)

This looks like its an issue elsewhere in the MSE stack.
Assignee: jyavenard → j
Attachment #8642431 - Flags: review?(jyavenard)
Comment on attachment 8642431 [details] [diff] [review]
Bug-1182946-Re-enable-webm-mediasource-tests.patch

Review of attachment 8642431 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/profiles/prefs_general.js
@@ +165,4 @@
>  
>  // Enable Media Source Extensions for testing
>  user_pref("media.mediasource.mp4.enabled", true);
> +user_pref("media.mediasource.webm.enabled", true);

We won't enable webm in mediasource at this stage.

It's been decided to be linux only to start with.

@@ +165,5 @@
>  
>  // Enable Media Source Extensions for testing
>  user_pref("media.mediasource.mp4.enabled", true);
> +user_pref("media.mediasource.webm.enabled", true);
> +user_pref("media.mediasource.format-reader.webm", true);

this is out of scope
Attachment #8642431 - Flags: review?(jyavenard) → review-
(In reply to Jean-Yves Avenard [:jya] from comment #8)
> Comment on attachment 8642431 [details] [diff] [review]
> Bug-1182946-Re-enable-webm-mediasource-tests.patch
> 
> Review of attachment 8642431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/profiles/prefs_general.js
> @@ +165,4 @@
> >  
> >  // Enable Media Source Extensions for testing
> >  user_pref("media.mediasource.mp4.enabled", true);
> > +user_pref("media.mediasource.webm.enabled", true);
> 
> We won't enable webm in mediasource at this stage.
> 
> It's been decided to be linux only to start with.

Ok only enabling mediasource for tests on linux for now.

> @@ +165,5 @@
> >  
> >  // Enable Media Source Extensions for testing
> >  user_pref("media.mediasource.mp4.enabled", true);
> > +user_pref("media.mediasource.webm.enabled", true);
> > +user_pref("media.mediasource.format-reader.webm", true);
> 
> this is out of scope

You want to run the webm mediasource tests with WebMReader? On the other hand might be best to get rid of media.mediasource.format-reader.webm and always use MediaFormatReader for MSE (filed Bug 1190748)
Attachment #8642431 - Attachment is obsolete: true
Attachment #8642930 - Flags: review?(jyavenard)
(In reply to Jan Gerber from comment #9)
> You want to run the webm mediasource tests with WebMReader? On the other
> hand might be best to get rid of media.mediasource.format-reader.webm and
> always use MediaFormatReader for MSE (filed Bug 1190748)

no.

but this bug is about enabling webm with the new MSE .
Not enabling webm with the old MSE which we'll have no one to properly test
Comment on attachment 8642930 [details] [diff] [review]
Bug-1182946-Re-enable-webm-mediasource-tests-on-Linux.patch

Review of attachment 8642930 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/profiles/prefs_general.js
@@ +166,5 @@
>  // Enable Media Source Extensions for testing
>  user_pref("media.mediasource.mp4.enabled", true);
> +#if defined(XP_LINUX)
> +user_pref("media.mediasource.webm.enabled", true);
> +#else

oh sorry my bad ; I misread, I thought you were enabling MSE for non-testing.

so yes, always enabling webm here is fine.
(In reply to Jean-Yves Avenard [:jya] from comment #10)
> (In reply to Jan Gerber from comment #9)
> > You want to run the webm mediasource tests with WebMReader? On the other
> > hand might be best to get rid of media.mediasource.format-reader.webm and
> > always use MediaFormatReader for MSE (filed Bug 1190748)
> 
> no.
> 
> but this bug is about enabling webm with the new MSE .
> Not enabling webm with the old MSE which we'll have no one to properly test

Sorry my bad, was under the impression media.mediasource.format-reader.webm
is needed to test the new MSE. Looking at it again, this was only used to use
MediaFormatReader with the old MSE stack.
- media.mediasource.webm.enabled is enabled for all tests and tests only run on linux.
- test_BufferingWait.html is disabled for now, created Bug 1190776 to enable it.
Attachment #8642930 - Attachment is obsolete: true
Attachment #8642930 - Flags: review?(jyavenard)
Attachment #8642952 - Flags: review?(jyavenard)
- run tests on all platforms.
- test_BufferingWait.html is disabled for now, created Bug 1190776 to enable it.
Attachment #8642952 - Attachment is obsolete: true
Attachment #8642952 - Flags: review?(jyavenard)
Attachment #8642964 - Flags: review?(jyavenard)
Comment on attachment 8642964 [details] [diff] [review]
Bug-1182946-Re-enable-webm-mediasource-tests.patch

Review of attachment 8642964 [details] [diff] [review]:
-----------------------------------------------------------------

let see how that go..

If you checked the original code, some were excluded on some platforms.

let see how it goes via a try run

::: dom/media/mediasource/test/mochitest.ini
@@ +79,4 @@
>  [test_SeekTwice_mp4.html]
>  skip-if = ((os == "win" && os_version == "5.1") || (os != "win" && os != "mac")) # Only supported on osx and vista+
>  [test_SetModeThrows.html]
> +skip-if = true

if the test isn't valid ; then it should be removed completely alltogether
Attachment #8642964 - Flags: review?(jyavenard) → review+
Blocks: 1191833
Looking at the results from

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=85f1ef18fcc1

All media-source web-platform-tests need to be updated too.
Here comes a new patch including web-platform-tests.
Attachment #8642964 - Attachment is obsolete: true
Attachment #8644420 - Flags: review?(jyavenard)
Comment on attachment 8644420 [details] [diff] [review]
Bug-1182946-Re-enable-webm-mediasource-tests.patch

Review of attachment 8644420 [details] [diff] [review]:
-----------------------------------------------------------------

The mochitest needs to be split from the webref changes.

Have karlt review the webref changes.

This looks very good for the webref though. Unfortunate however, that webm working now hides h264 failures.

::: testing/web-platform/meta/media-source/mediasource-is-type-supported.html.ini
@@ +1,4 @@
>  [mediasource-is-type-supported.html]
>    type: testharness
>    prefs: [media.mediasource.enabled:true]
> +  [Test invalid MIME format "video/webm"] # Bug 1191833

did you test that those changes work?

you can't have comments there unfortunately. Only with "disabled:"

::: testing/web-platform/meta/media-source/mediasource-seek-during-pending-seek.html.ini
@@ +4,5 @@
>    disabled:
>      if os == "mac": https://bugzilla.mozilla.org/show_bug.cgi?id=1183523
>      if (os == "win") and (version != "5.1.2600"): https://bugzilla.mozilla.org/show_bug.cgi?id=1183523
> +    if (os == "linux"): https://bugzilla.mozilla.org/show_bug.cgi?id=1183523
> +    if (os == "win") and (version == "5.1.2600"): https://bugzilla.mozilla.org/show_bug.cgi?id=1183523

so that means it's now to be disabled on all platforms ; don't need to have all platforms listed ; though I guess it doesn't really matter
Attachment #8644420 - Flags: review?(jyavenard) → feedback?(karlt)
Comment on attachment 8644420 [details] [diff] [review]
Bug-1182946-Re-enable-webm-mediasource-tests.patch

> [test_SetModeThrows.html]
>-skip-if = true # bug 1182946
>+skip-if = true

Link to a bug tracking this issue.

>+++ b/testing/web-platform/meta/media-source/mediasource-seek-during-pending-seek.html.ini
>@@ -4,13 +4,5 @@
>   disabled:
>     if os == "mac": https://bugzilla.mozilla.org/show_bug.cgi?id=1183523
>     if (os == "win") and (version != "5.1.2600"): https://bugzilla.mozilla.org/show_bug.cgi?id=1183523

>+    if (os == "linux"): https://bugzilla.mozilla.org/show_bug.cgi?id=1183523
>+    if (os == "win") and (version == "5.1.2600"): https://bugzilla.mozilla.org/show_bug.cgi?id=1183523

These look like they can be unified.
I suspect there is no platform where it is not disabled.

(In reply to Jean-Yves Avenard [:jya] from comment #17)
> The mochitest needs to be split from the webref changes.

I don't think that is possible because prefs_general.js affects both web platform and mochitest.
Attachment #8644420 - Flags: feedback?(karlt) → feedback+
(In reply to Karl Tomlinson (ni?:karlt) from comment #18)
> Comment on attachment 8644420 [details] [diff] [review]
> Bug-1182946-Re-enable-webm-mediasource-tests.patch
> 
> > [test_SetModeThrows.html]
> >-skip-if = true # bug 1182946
> >+skip-if = true
> 
> Link to a bug tracking this issue.

This test was actually outdated, testing now that it does not throw.

> >+++ b/testing/web-platform/meta/media-source/mediasource-seek-during-pending-seek.html.ini
> >@@ -4,13 +4,5 @@
> >   disabled:
> >     if os == "mac": https://bugzilla.mozilla.org/show_bug.cgi?id=1183523
> >     if (os == "win") and (version != "5.1.2600"): https://bugzilla.mozilla.org/show_bug.cgi?id=1183523
> 
> >+    if (os == "linux"): https://bugzilla.mozilla.org/show_bug.cgi?id=1183523
> >+    if (os == "win") and (version == "5.1.2600"): https://bugzilla.mozilla.org/show_bug.cgi?id=1183523
> 
> These look like they can be unified.
> I suspect there is no platform where it is not disabled.

true, disabling on all platforms
Attachment #8644420 - Attachment is obsolete: true
Attachment #8644855 - Flags: review?(jyavenard)
upload new patch...
Attachment #8644855 - Attachment is obsolete: true
Attachment #8644855 - Flags: review?(jyavenard)
Attachment #8644858 - Flags: review?(jyavenard)
Comment on attachment 8644858 [details] [diff] [review]
Bug-1182946-Re-enable-webm-mediasource-tests.patch

Review of attachment 8644858 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasource/test/test_SetModeThrows.html
@@ +21,5 @@
>      ok("true", "Setting to segments does not throw");
>      try {
>        sb.mode = "sequence";
> +      ok("true", "Setting to segments does not throw");
> +    } catch (e) { ok(/supported/.test(e), "Should not throw setting mode to sequence: " + e); }

that line doesn't sound right...

how would that fail if it did throw?
Attachment #8644858 - Flags: review?(jyavenard) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #21)
> ::: dom/media/mediasource/test/test_SetModeThrows.html
> @@ +21,5 @@
> >      ok("true", "Setting to segments does not throw");
> >      try {
> >        sb.mode = "sequence";
> > +      ok("true", "Setting to segments does not throw");
> > +    } catch (e) { ok(/supported/.test(e), "Should not throw setting mode to sequence: " + e); }
> 
> that line doesn't sound right...
> 
> how would that fail if it did throw?

lets try this again, now with ok(false, "Should not throw setting mode to sequence: " + e);
Attachment #8644858 - Attachment is obsolete: true
Attachment #8644868 - Flags: review+
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=12625937&repo=mozilla-inbound
Flags: needinfo?(j)
looks like those tests are a bit racy. in any case the reason seams to be that the first chunk attached (up to the end of the first media segment) was [0, 25223] and not [0, 25523]. The first media segment ends at 25523 though. Updating all tests that used 25223 instead of 25523.
Flags: needinfo?(j)
Attachment #8645035 - Flags: review?(jyavenard)
the patch updates all tests to use 25523 instead of 25223 of cause.
Attachment #8645035 - Flags: review?(jyavenard) → review+
(In reply to Jan Gerber from comment #28)
> Created attachment 8645035 [details] [diff] [review]
> Bug-1182946-end-of-first-media-segment-is-25523.patch
> 
> looks like those tests are a bit racy. in any case the reason seams to be
> that the first chunk attached (up to the end of the first media segment) was
> [0, 25223] and not [0, 25523]. The first media segment ends at 25523 though.
> Updating all tests that used 25223 instead of 25523.

Why would that be racy?

Partial media segment should be perfectly acceptable ; and I think should probably safe to miss just 300 bytes of it.

Couldn't this indicate an issue with the webm demuxer not properly handling partial media segment?

The old MSE never had issues with those tests.

try looks good.

But I think we should investigate
Flags: needinfo?(j)
Blocks: 1192517
https://hg.mozilla.org/mozilla-central/rev/997748793428
https://hg.mozilla.org/mozilla-central/rev/fcf7077de8fe
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Note that bug comments in ini files are removed on updates: https://hg.mozilla.org/integration/mozilla-inbound/diff/dcdf6e96e9a0/testing/web-platform/meta/media-source/mediasource-is-type-supported.html.ini

You can use

[test name]
  bug: ...

which will stay around.
Blocks: 1194632
(In reply to Jean-Yves Avenard [:jya] from comment #31)
> (In reply to Jan Gerber from comment #28)
> > Created attachment 8645035 [details] [diff] [review]
> > Bug-1182946-end-of-first-media-segment-is-25523.patch
> > 
> > looks like those tests are a bit racy. in any case the reason seams to be
> > that the first chunk attached (up to the end of the first media segment) was
> > [0, 25223] and not [0, 25523]. The first media segment ends at 25523 though.
> > Updating all tests that used 25223 instead of 25523.
> 
> Why would that be racy?
> 
> Partial media segment should be perfectly acceptable ; and I think should
> probably safe to miss just 300 bytes of it.
> 
> Couldn't this indicate an issue with the webm demuxer not properly handling
> partial media segment?
> 
> The old MSE never had issues with those tests.
> 
> try looks good.
> 
> But I think we should investigate

Looks indeed like there is a problem with partial segments.
Patch and some more details in Bug 1192517
Flags: needinfo?(j)
Is there a reason why test_EndOfStream.html didn't get re-enabled? https://hg.mozilla.org/mozilla-central/rev/7bf6ab248f72#l1.20
Flags: needinfo?(jyavenard)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #36)
> Is there a reason why test_EndOfStream.html didn't get re-enabled?
> https://hg.mozilla.org/mozilla-central/rev/7bf6ab248f72#l1.20

it is only disabled on android. though I see another that should have been re-enabled will open a new bug for it
Flags: needinfo?(jyavenard)
Depends on: 1314212
You need to log in before you can comment on or make changes to this bug.