804875 - videoObject.videoWidth returns outdated information

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Dale Harvey (:daleharvey)]

I have a videoObject being resused to generate screenshots for various videos, when I set the source of the videoObject to an audio file (.ogg), onmetadataloaded is still called and the videoObject.videoWidth is that of the previous video. 

If I use a fresh videoObject then the videoWidth is 0

Comment 1

[Matthew Gregan [:kinetik]]

Same issue with mozPaintedFrames and mozFrameDelay.  We also don't update the element's size and we continue to paint the old video frame if the new src is audio only.

Comment 2

[Paul Adenot (:padenot)]

Attachment: Reset media element when loading a new src. r=

This fixes what comment 0 and comment 1 describe, and is mostly green on try
(needs a patch for bug 825325 to make the test work on Fennec).

Comment 3

[Paul Adenot (:padenot)]

Attachment: Tests. r=

Test for this change.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 696404 [details] [diff] [review]
Reset media element when loading a new src. r=

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

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +737,5 @@
>    mIsRunningLoadMethod = false;
>    return NS_OK;
>  }
>  
> +void nsHTMLMediaElement::ResetState()

Normally we just reset state that needs to be reset upon a new load inside nsHTMLMediaElement::AbortExistingLoads(). Not the most obvious name I guess.

Comment 5

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55e93d1fa972
https://hg.mozilla.org/integration/mozilla-inbound/rev/f14639a3461e

Comment 6

[Ed Morley [:emorley]]

Push backed out for Windows pgo-only mochitest-1 timeouts in media tests, since the backout of just 1abf4c88f8f1 didn't work (see bug 793274 comment 12 for example logs):
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2912b7e727a

Comment 7

[Ed Morley [:emorley]]

Before this was backed out, we were getting this intermittent-failure in test_reset_src.html:
https://tbpl.mozilla.org/php/getParsedLog.php?id=18829054&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=18851231&tree=Mozilla-Inbound

Comment 8

[Ed Morley [:emorley]]

(Please can the intermittent test failure be fixed before this relands)

Comment 9

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49eaea676d49
https://hg.mozilla.org/integration/mozilla-inbound/rev/c996159eb596

I bisected the queue on try, those patches are safe.

(also fixed the failing tests).

Comment 10

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/49eaea676d49
https://hg.mozilla.org/mozilla-central/rev/c996159eb596

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Causes the getUserMedia tests to fail when Paused or when Stopped and re-started.  See bug 836011

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Backed out for now until regressions are handled, rs=kinetik
Did a local test to verify the backout.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bc1aa6145ed

Comment 13

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

(In reply to Randell Jesup [:jesup] from comment #12)
> Backed out for now until regressions are handled, rs=kinetik
> Did a local test to verify the backout.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3bc1aa6145ed

https://hg.mozilla.org/mozilla-central/rev/3bc1aa6145ed

Comment 14

[Paul Adenot (:padenot)]

Attachment: Reset all the members of the VideoFrameComtainer in the Reset method. r=

That's what you get when you don't reset all the things you need to reset in the
::Reset method.

Basically, the mIntrinsicSize member was not reset, even though we invalidated
the frame. Then the new video was loaded, the element sets it's copy of the
dimensions to {-1, -1}, and then the first frame of the video got set. Because the
old dimensions were still in the VideoFrame, we did not notify the element of
the new dimensions, invalidate, and so on.

I also plan to land the patch you made to the test.

I could not find another camera, so I can't make sure it works in the multiple
camera getUserMedia case, but I'm pretty confident.

Comment 15

[Matthew Gregan [:kinetik]]

Comment on attachment 708565 [details] [diff] [review]
Reset all the members of the VideoFrameComtainer in the Reset method. r=

Thanks!

I verified that http://mozilla.github.com/webrtc-landing/gum_test.html works per bug 836011.  I only tested with 1 camera, but pause and stop/restart works fine.

Comment 16

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75a6ebf32b41
https://hg.mozilla.org/integration/mozilla-inbound/rev/e976b6bc1477
https://hg.mozilla.org/integration/mozilla-inbound/rev/37900f7d7bd9

Comment 17

[Paul Adenot (:padenot)]

*Shakes fist about MSVC*

https://hg.mozilla.org/integration/mozilla-inbound/rev/f01a7922583b

Comment 18

[Paul Adenot (:padenot)]

Changed a couple of messages to be proper English, and fixed an orange. 

https://hg.mozilla.org/integration/mozilla-inbound/rev/6320dce4900e

Comment 19

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/75a6ebf32b41
https://hg.mozilla.org/mozilla-central/rev/e976b6bc1477
https://hg.mozilla.org/mozilla-central/rev/37900f7d7bd9
https://hg.mozilla.org/mozilla-central/rev/f01a7922583b
https://hg.mozilla.org/mozilla-central/rev/6320dce4900e

Comment 20

[Dale Harvey (:daleharvey)]

Am I right that this is backed out / reopened? 

https://github.com/mozilla/mozilla-central/commit/6588f17f5b4e1d672ae873217256166f6e8b1bdd

Comment 21

[Matthew Gregan [:kinetik]]

It was backed out (see comments 12-13), then relanded (see comments 16-19).

Comment 22

[Broton]

If I have two camera, and I toggle camera, the mMediaSize of nsHTMLMediaElement will be reset to (-1,-1) after toggle camera. Does it works correctly in this case?

Comment 23

[Paul Adenot (:padenot)]

Broton, the size will be set after the second camera's video is loaded in the nsHTMLMediaElement. If you don't observe this, please file a bug and CC me.