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

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.

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.

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

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

Thanks Alice!

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 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.

(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.

(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.

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

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. :|

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

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

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

(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

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