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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[David Humphrey (:humph)]

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 - http://audioscene.org/scene-files/humph/VideoShaderSandbox/video_copy_bug.html

WebGL - http://audioscene.org/scene-files/humph/VideoShaderSandbox/video_shader_sandbox.html

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Is this a regression from 3.6?

Comment 2

[David Humphrey (:humph)]

To test on 3.6, here is the same thing with ogv instead of webm.  We are seeing the same glitching:

2D - http://audioscene.org/scene-files/humph/VideoShaderSandbox/video_copy_bug_ogv.html

WebGL - http://audioscene.org/scene-files/humph/VideoShaderSandbox/video_shader_sandbox_ogv.html

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Why does 3.6 glitch, though?

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Not sure, but I'd guess a similar type of bug..

Comment 6

[Joe Drew (not getting mail)]

Want it fixed, but can release without the fix (considering we had it in 3.6).

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: recycle buffer just before getting new buffer

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.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

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 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/3769e11d18d6

Leaving open until we're done discussing The Right Fix.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Closing this; we've been using this solution for 6 months now without report of glitches AFAIK.