Closed Bug 604571 Opened 12 years ago Closed 11 years ago

Copying frames from <video> to <canvas> with drawImage glitches


(Core :: Graphics, defect)

Not set



Tracking Status
blocking2.0 --- -


(Reporter: humph, Assigned: bjacob)


(Whiteboard: [approved-patches-landed])


(1 file)

Doing some demos that modify frames in a <video> element using <canvas>, we're seeing the <canvas> glitch (e.g., showing frames out of order sometimes).  We're seeing this on today's nightly on 10.6.  Here are two examples, one with a 2d canvas, and one with WebGL and some shader code.  Both have the same issue:

2D -

WebGL -

Various people asked if this was bug 602659.  It looks very much like it, but the video is fine, and the canvas copy is all that is glitching; therefore I don't think it's the same thing.

Based on conversations in irc, this isn't an issue on all platforms, but I can only test 10.6 here.
Is this a regression from 3.6?
blocking2.0: --- → ?
To test on 3.6, here is the same thing with ogv instead of webm.  We are seeing the same glitching:

2D -

WebGL -
So the way this works is that the data buffers for the video frame images are recycled, and they're marked as no-longer-in-use as soon as the frame is uploaded to a texture.  When canvas goes and grabs the data, it gets that "old" pointer, which has likely already been reused by the decoder to fill in the data for a newer frame than what's being displayed.

A fix would be to make GL layers not recycle the current frame's data until the next frame is about to be uploaded, though that might increase memory usage by 1 frame's worth; should be easy, at the end of PlanarYCbCrImageOGL::UpdateTextures thre's a call to recycle -- that should happen elsewhere, perhaps in the call to SetImage (or whatever it's called) to change the current image.
Why does 3.6 glitch, though?
Not sure, but I'd guess a similar type of bug..
Want it fixed, but can release without the fix (considering we had it in 3.6).
blocking2.0: ? → -
Comment 3 sounds like it's so easy to fix, I'll give this a go even though that's not a "blocker": the #audio team really needs it.
This fixes it for me.

I have moved it to just before where we get a new buffer, is that The Right Fix?

Question: should we lock a mutex around this buffer swapping?

Question 2: can the mBufferSize really change from frame to frame? If no, then my patch is stupid and should just reuse the mBuffer, I guess.
Assignee: nobody → bjacob
Attachment #487034 - Flags: review?(vladimir)
Hmm.. that would work, but I think there's a race there -- SetData is called on the decoder thread, so there's a period here where a caller could still get a pointer to the now-recycled data.  I have no idea how to get around that, though; not without introducing some kind of refcounting scheme where a caller can hold on to a buffer/set of buffers outside of the normal image scheme here.  But, it's no worse than the current situation, and is arguably much better, so let's do this.

The size can theoretically change; that's why we have the whole recycling buffer mechanism.  It'll effectively cause you to reuse the buffer if it doesn't change.
Need more detail in order to be able to understand this:

  * is PlanarYCbCrImageOGL::SetData() called on the "current" image as returned by ImageContainerOGL::GetCurrentAsSurface() ?

  * if no, then what is the race?

  * if yes, I gues that we need to lock a mutex to prevent ImageContainerOGL::GetCurrentAsSurface() from running concurrently with PlanarYCbCrImageOGL::SetData(). Can you detail how you would do this? ImageContainerOGL::GetCurrentAsSurface() is already locking a mutex, but that is private to ImageContainerOGL. Should we add a new mutex in PlanarYCbCrImageOGL and lock it in both PlanarYCbCrImageOGL::SetData() and ImageContainerOGL::GetCurrentAsSurface()?
Comment on attachment 487034 [details] [diff] [review]
recycle buffer just before getting new buffer

I think this is ok; I'm wondering why we shouldn't just reuse the buffer if it's the same size, without going through recycling, but it doesn't really matter.
Attachment #487034 - Flags: review?(vladimir)
Attachment #487034 - Flags: review+
Attachment #487034 - Flags: approval2.0+

Leaving open until we're done discussing The Right Fix.
Whiteboard: [approved-patches-landed]
Closing this; we've been using this solution for 6 months now without report of glitches AFAIK.
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.