Closed Bug 804875 Opened 7 years ago Closed 7 years ago

videoObject.videoWidth returns outdated information

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: daleharvey, Assigned: padenot)

References

Details

Attachments

(3 files)

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
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.
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → paul
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).
Attachment #696404 - Flags: review?(kinetik)
Attached patch Tests. r=Splinter Review
Test for this change.
Attachment #696405 - Flags: review?(kinetik)
Depends on: 825325
Attachment #696405 - Flags: review?(kinetik) → review+
Attachment #696404 - Flags: review?(kinetik) → review+
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.
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
(Please can the intermittent test failure be fixed before this relands)
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).
https://hg.mozilla.org/mozilla-central/rev/49eaea676d49
https://hg.mozilla.org/mozilla-central/rev/c996159eb596
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Blocks: 835075
No longer blocks: 835075
Depends on: 835075
Causes the getUserMedia tests to fail when Paused or when Stopped and re-started.  See bug 836011
Depends on: 836011
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla21 → ---
(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
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.
Attachment #708565 - Flags: review?(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.
Attachment #708565 - Flags: review?(kinetik) → review+
Changed a couple of messages to be proper English, and fixed an orange. 

https://hg.mozilla.org/integration/mozilla-inbound/rev/6320dce4900e
Am I right that this is backed out / reopened? 

https://github.com/mozilla/mozilla-central/commit/6588f17f5b4e1d672ae873217256166f6e8b1bdd
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
It was backed out (see comments 12-13), then relanded (see comments 16-19).
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
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?
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.
Blocks: 844722
Depends on: 842535
You need to log in before you can comment on or make changes to this bug.