844676 - gUM <video> surface not available immediately in dual-GPU MacBooks

Status: RESOLVED DUPLICATE of bug 879717

(Core :: WebRTC: Audio/Video, defect)

Description

[Reuben Morais [:reuben]]

STR:

1) On a dual-GPU MacBook, go to http://idevelop.github.com/ascii-camera/
2) Allow access to the camera in the gUM dialog

Results:
NS_ERROR_NOT_AVAILABLE: Component is not available @ http://idevelop.github.com/ascii-camera/script/camera.js:48
https://hg.mozilla.org/mozilla-central/file/0d0071a94fe9/content/canvas/src/CanvasRenderingContext2D.cpp#l2928

Even if I wait for canplay/loadedmetadata/loadeddata before calling draw(), the surface is still not available (GPU switching can take a very long time). The only (ugly) wait to fix it is to wrap the drawImage call in a try-catch block and keep retrying until it works.

Expected results:
We should either draw nothing, or only fire canplay after the surface is available.

Comment 1

[Jason Smith [:jsmith]]

I can reproduce on Win 7 as well. I recall us having bugs on drawImage in the past with videos using local media streams derived from gUM.

Comment 2

[Jason Smith [:jsmith]]

Works fine on Chrome.

Comment 3

[Jason Smith [:jsmith]]

Ah, here's the old bug that talked about some of what we're seeing here - bug 771833.

The discussion talks about the need to wait for canplay or loadeddata to fire. But why does this work in Chrome then, but fail in Firefox?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know, but it's still not according to the spec AFAIK.

I think we should probably draw nothing, though.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix the canvas code

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This isn't going to fix the Optimus issue. That's really a separate bug --- a gfx bug. I'm not sure how to fix that.

Comment 7

[Nick Cameron [:nrc]]

Comment on attachment 718216 [details] [diff] [review]
fix the canvas code

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

::: layout/base/nsLayoutUtils.cpp
@@ +4652,5 @@
>  
>    uint16_t readyState;
> +  if (NS_FAILED(aElement->GetReadyState(&readyState)) ||
> +      readyState == nsIDOMHTMLMediaElement::HAVE_NOTHING ||
> +      readyState == nsIDOMHTMLMediaElement::HAVE_METADATA) {

Was this a bug before and you have fixed it? (The new version feels right, but just to check that we are meant to return if GetReadyState fails).

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

GetReadyState can't really fail. This is just a tiny cleanup.

Comment 9

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

Once this lands, can I have an assessment of how large a problem this would be in a release?  I.e. what/how many macs are affected, how bad is it for applications, what's the risk?  (I.e. a preview to know if this should be slated for Beta uplift; I suspect we'll at least want Aurora).  Thanks!

Comment 10

[Jason Smith [:jsmith]]

Need Roc's input if this blocks or not. Marking ? as such.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We really need to split this into two bugs. I think we should make this bug about the Optimus issue (assuming the Web page is fixed to wait for canplaythrough before starting to use drawImage). I'll file another bug about when canvas drawImage should throw an exception and move my patch there. I don't think that bug should block. I'm not sure about this one.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Filed bug 847357.

Comment 13

[Maire Reavy [:mreavy]]

I would like to get this fixed, but I don't think this should block our first release of gUM.  I think we can note this bug in the release notes for Fx20 as a known bug that we are working to fix.

Even though I don't think this should block, I'd love to get someone from gfx to look at this as soon as possible and assess when we could get a fix.  

I've added Joe Drew to this bug.  Joe -- is this a bug you can look at yourself?  If not, can you recommend someone who can?  Thanks.

Comment 14

[Jason Smith [:jsmith]]

comment 13 reads off as:

1. relnote nom for 20
2. track this for 21 - let's get this into Aurora 21 for the first stablization followup release of gUM (important, but not blocking v1, so not needed on Beta 20)

Comment 15

[bhavana bajaj [:bajaj]]

Adding ni on Joe to help with request in comment 13.Joe, please let us know if this is something you can help us with or recommend someone here . Thanks !

Comment 16

[Joe Drew (not getting mail)]

It's not clear to me what "the Optimus issue" is here. Where does drawImage fail? (I have no Optimus machine.)

Comment 17

[Jason Smith [:jsmith]]

Going to redirect to the reporter here since he's got the machine available right now. Reuben - Can you address comment 16?

Comment 18

[Reuben Morais [:reuben]]

(In reply to Joe Drew (:JOEDREW! \o/) (Unburying, use needinfo) from comment #16)
> It's not clear to me what "the Optimus issue" is here. Where does drawImage
> fail? (I have no Optimus machine.)

http://reubenmorais.github.io/ascii-camera/

Note that I changed it to only call drawImage after the video's canplaythrough event, but the drawImage still fails with NS_ERROR_NOT_AVAILABLE.

Comment 19

[Joe Drew (not getting mail)]

Sorry, I meant "where does it fail in CanvasRenderingContext2D::DrawImage".

Comment 20

[Reuben Morais [:reuben]]

http://hg.mozilla.org/mozilla-central/file/e8bbd291396a/content/canvas/src/CanvasRenderingContext2D.cpp#l2964

http://pastebin.mozilla.org/2300266

Comment 21

[bhavana bajaj [:bajaj]]

joe, can you please help with the next steps here given comment #20?

Comment 22

[Joe Drew (not getting mail)]

Thanks Reuben - another question: what happens in SurfaceFromElement? At some point we should have assigned to res.mSurface, but we didn't for some reason.

(Oh and please needinfo? me so I notice! :) )

Comment 23

[Reuben Morais [:reuben]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #22)
> Thanks Reuben - another question: what happens in SurfaceFromElement? At
> some point we should have assigned to res.mSurface, but we didn't for some
> reason.

The HTMLVideoElement::GetImageContainer call fails because the following check HTMLMediaElement::GetVideoFrameContainer fails:

if (mReadyState >= nsIDOMHTMLMediaElement::HAVE_METADATA &&
    mMediaSize == nsIntSize(-1, -1)) {
  return nullptr;
}

readyState is HAVE_ENOUGH_DATA, but the size is -1x-1.

mVideoFrameContainer does exist there, but it doesn't have a valid surface.

UpdateMediaSize is then called:

#0 mozilla::dom::HTMLMediaElement::UpdateMediaSize (this=0x13425d160) at /content/html/content/src/HTMLMediaElement.cpp:3189
#1 mozilla::VideoFrameContainer::Invalidate (this=0x143b6b410) at /content/media/VideoFrameContainer.cpp:140
#2 nsRunnableMethodImpl<void (mozilla::VideoFrameContainer::*)(), true>::Run (this=<value temporarily unavailable, due to optimizations>) at nsThreadUtils.h:350
#3 nsThread::ProcessNextEvent (this=0x107d03b90, mayWait=<value temporarily unavailable, due to optimizations>, result=0x7fff5fbfd777) at /xpcom/threads/nsThread.cpp:627
#4 NS_ProcessPendingEvents (thread=<value temporarily unavailable, due to optimizations>, timeout=20) at nsThreadUtils.cpp:188
#5 nsBaseAppShell::NativeEventCallback (this=0x10012d120) at /widget/xpwidgets/nsBaseAppShell.cpp:97
#6 nsAppShell::ProcessGeckoEvents (aInfo=0x10012d120) at /widget/cocoa/nsAppShell.mm:387

So it seems to me that the video source is still being loaded, so this shouldn't be throwing. Setting res.mIsStillLoading in that case seems appropriate, and it does fix the problem.

Comment 24

[Reuben Morais [:reuben]]

(In reply to Reuben Morais [:reuben] from comment #23)
> So it seems to me that the video source is still being loaded, so this
> shouldn't be throwing. Setting res.mIsStillLoading in that case seems
> appropriate, and it does fix the problem.

To make this clearer, setting it in the case of an invalid ImageContainer in SurfaceFromElement. I don't know how correct that would be, but we have plenty of experts in this bug who can answer that :)

Comment 25

[Joe Drew (not getting mail)]

Roc, I briefly looked at the code in SurfaceFromElement, but it looks somewhat intentional, so I don't want to add the mIsStillLoading without knowing HTMLVideoElement better. Perhaps someone on the media team does :)

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Adding mIsStillLoading = true there sounds right to me.

Comment 27

[bhavana bajaj [:bajaj]]

Giving we have only one beta left for Fx21, I am wontfixing the issue at this point.

We still have ~2 weeks for Fx22 on aurora and any uplift that may need to happen when Fx22 is on aurora will be great.

Adding needsinfo on Joe, to make sure we are on track for Fx22 which is the expectation here else please let us know and we would untrack at this point of time.

Comment 28

[Joe Drew (not getting mail)]

Attachment: set mIsStillLoading if we can't get an ImageContainer

Like this, I presume

Comment 29

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5878a9a7c540

Comment 30

[Joe Drew (not getting mail)]

And backed out, because "Surely this could not cause test failures" is apparently not a substitute for running the tests http://hg.mozilla.org/integration/mozilla-inbound/rev/153993320a45

Comment 31

[Joe Drew (not getting mail)]

Paul, this patch causes:

08:32:34 INFO - 203700 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_reset_src.html | We should not succeed to draw a video frame on the canvas.

Do you have any ideas?

Comment 32

[Paul Adenot (:padenot)]

So, the test is trying to draw an <video> tag that has no video track to a canvas, to ensure that it does not work. The test expects an exception to be thrown, not sure if this is the expected behavior. This behavior is changed by the first patch. Fell free to change the test if we should not throw in this case.

Comment 33

[Joe Drew (not getting mail)]

From HTML5's documentation on drawWindow:

> if the image argument is an HTMLVideoElement object whose readyState attribute is either
> HAVE_NOTHING or HAVE_METADATA, then return bad and abort these steps.

At this point I'm in way, way over my head; Matthew, can you or your designate figure out what the right thing to do here is?

Comment 34

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

Andreas - do you think this is still relevant? - or has this become moot, or have things so changed that one should just check it again on the right hardware before even trying to answer the question?

Comment 35

[Andreas Pehrson [:pehrsons] (back on Aug 5)]

This should have been fixed by bug 879717. I'd even say this is a dupe of 879717 (and not about dual GPUs at all).

With 879717 we only go to HAVE_CURRENT_DATA when the image container have an image. So mIsStillLoading is now true until there's an actual image and we no longer have a gap where the canvas element can complain.