937429 - VideoDocument shows no video

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Ralph Giles (:rillian)]

In recent nightly on my retina MacBook, VideoDocument shows no video. I get a narrow back strip across the background texture which sometimes shows a loading spinner before disappearing.

The same video files loaded in an html page <video> element play normally.

Tested with https://people.xiph.org/~giles/2006/mdis_earth.ogg and https://people.mozilla.org/~rgiles/2013/demo.webm

Comment 1

[Ralph Giles (:rillian)]

My local build spews a lot of:

WARNING: could not find a compositor to schedule composition: file /Users/giles/firefox/gfx/layers/ipc/CompositableTransactionParent.cpp, line 225

to the terminal.

Comment 2

[Ralph Giles (:rillian)]

Chris Pierce was able to confirm on Windows, including with an mp4 file. His earlier nightly from November 4 worked, so it's a regression in the last week, 2013 Nov 4-11.

Comment 3

[Ralph Giles (:rillian)]

We suspect the reftests didn't catch this because they test html pages, not the built-in VideoDocument. :/

Comment 4

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/21b77163bf9f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131107041709
Bad:
http://hg.mozilla.org/mozilla-central/rev/f73dd492c34c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131107052509
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=21b77163bf9f&tochange=f73dd492c34c



Regression window(fx)
Good:
http://hg.mozilla.org/integration/fx-team/rev/c17073bda80b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131106182613
Bad:
http://hg.mozilla.org/integration/fx-team/rev/f9a5108cf2b2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131106200125
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=c17073bda80b&tochange=f9a5108cf2b2

Regressed by:
f9a5108cf2b2	Jared Wein — Bug 704326 - Standalone audio files should have different style so they don't look awkward. r=smaug,dolske

Comment 5

[Ralph Giles (:rillian)]

Thanks Alice!

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 831774 [details] [diff] [review]
Patch

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

::: toolkit/content/tests/widgets/test_videocontrols_standalone.html
@@ +14,5 @@
> +
> +/*
> + * Positions of the UI elements, relative to the upper-left corner of the
> + * <video> box.
> + */

This comment can be removed.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

https://tbpl.mozilla.org/?tree=Try&rev=82d4a48d3ebe

Comment 11

[Justin Dolske [:Dolske]]

Comment on attachment 831774 [details] [diff] [review]
Patch

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

r- for the test, and I'd sorta like to understand what I commented on for the XBL change.

::: toolkit/content/tests/widgets/test_videocontrols_standalone.html
@@ +19,5 @@
> +const videoWidth = 320;
> +const videoHeight = 240;
> +
> +var popup = window.open("seek_with_sound.ogg");
> +popup.addEventListener("load", function onLoad() {

I thought media elements stopped firing |load| (or blocking the document's |load|). So this would seem racy.

I think you want to wait for the element's |loadedmetadata| event before trying to do anything with it.

@@ +21,5 @@
> +
> +var popup = window.open("seek_with_sound.ogg");
> +popup.addEventListener("load", function onLoad() {
> +  popup.removeEventListener("load", onLoad);
> +  var video = popup.document.body.firstChild;

document.getElementsByTagName("video")[0] would be a little more future-proof? But meh.

@@ +33,5 @@
> +  }
> +});
> +
> +function runTestVideo() {
> +  var video = popup.document.body.firstChild;

Maybe make a helper for this? Or just grab it once globally?

::: toolkit/content/widgets/videocontrols.xml
@@ +349,5 @@
> +                        this.video.style.width = "66%";
> +                    } else {
> +                        this.video.style.removeProperty("height");
> +                        this.video.style.removeProperty("width");
> +                    }

This fix implies we're first setting |isAudioOnly = true|, and later setting |isAudioOnly = false|... Where/when is the first call happening? That seems like the root bug, since loading a standalone video file shouldn't ever be doing that (setting isAudioOnly=true).

I suppose this is still fine as belt-and-suspenders, but seems like it just shouldn't be needed in the first place.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Justin Dolske [:Dolske] from comment #11)
> ::: toolkit/content/tests/widgets/test_videocontrols_standalone.html
> @@ +19,5 @@
> > +const videoWidth = 320;
> > +const videoHeight = 240;
> > +
> > +var popup = window.open("seek_with_sound.ogg");
> > +popup.addEventListener("load", function onLoad() {
> 
> I thought media elements stopped firing |load| (or blocking the document's
> |load|). So this would seem racy.
> 
> I think you want to wait for the element's |loadedmetadata| event before
> trying to do anything with it.

This shouldn't be racy, because in this load event listener the test checks to see if the video is playing. If it isn't already playing it adds an event listener for the "play" event, and then begins the test when the video starts playing.

> ::: toolkit/content/widgets/videocontrols.xml
> @@ +349,5 @@
> > +                        this.video.style.width = "66%";
> > +                    } else {
> > +                        this.video.style.removeProperty("height");
> > +                        this.video.style.removeProperty("width");
> > +                    }
> 
> This fix implies we're first setting |isAudioOnly = true|, and later setting
> |isAudioOnly = false|... Where/when is the first call happening? That seems
> like the root bug, since loading a standalone video file shouldn't ever be
> doing that (setting isAudioOnly=true).

I'm looking in to this.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Justin Dolske [:Dolske] from comment #11)
> ::: toolkit/content/widgets/videocontrols.xml
> @@ +349,5 @@
> > +                        this.video.style.width = "66%";
> > +                    } else {
> > +                        this.video.style.removeProperty("height");
> > +                        this.video.style.removeProperty("width");
> > +                    }
> 
> This fix implies we're first setting |isAudioOnly = true|, and later setting
> |isAudioOnly = false|... Where/when is the first call happening? That seems
> like the root bug, since loading a standalone video file shouldn't ever be
> doing that (setting isAudioOnly=true).
> 
> I suppose this is still fine as belt-and-suspenders, but seems like it just
> shouldn't be needed in the first place.

I had figured this out before but forgot in the mean time. It's not that we are setting isAudioOnly=true, in fact we don't do that in the case of an actual video being referenced. But we do set isAudioOnly=false, which without this patch it still resizes the video as though it was audio-only.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

All green on tryserver: https://tbpl.mozilla.org/?tree=Try&rev=edc88d10ebe8

Comment 17

[Justin Dolske [:Dolske]]

Comment on attachment 8334699 [details] [diff] [review]
Patch v1.1

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

Seems like the test would be simpler if it just listened for loadedmetadata directly, so that it always had the same code flow.

Derp on me for not understanding the simple isAudioOnly fix. :|

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/b2a0444bc991

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Backed out the previous push due to timeouts on Linux:
https://hg.mozilla.org/integration/fx-team/rev/61e6df7c2637

Relanded with the patch that was originally pushed to tryserver:
https://hg.mozilla.org/integration/fx-team/rev/fae487ae86fc

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Backed out due to expected Android failures (though not confirmed yet!), 
https://hg.mozilla.org/integration/fx-team/rev/30e78558a271

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

The previous backout (comment #20) was uncalled for, relanded:
https://hg.mozilla.org/integration/fx-team/rev/0b84a9bee07a

Comment 22

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jared Wein [:jaws] from comment #21)
> The previous backout (comment #20) was uncalled for

No it wasn't. Backed out.
https://hg.mozilla.org/integration/fx-team/rev/7b13884d6ced

https://tbpl.mozilla.org/php/getParsedLog.php?id=30964743&tree=Fx-Team

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Fixed test on Android,
https://hg.mozilla.org/integration/fx-team/rev/baaf5eafdf51

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/baaf5eafdf51