Last Comment Bug 604571 - Copying frames from <video> to <canvas> with drawImage glitches
: Copying frames from <video> to <canvas> with drawImage glitches
Status: RESOLVED FIXED
[approved-patches-landed]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Mac OS X
-- normal (vote)
: ---
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-14 20:02 PDT by David Humphrey (:humph)
Modified: 2011-10-07 09:56 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
recycle buffer just before getting new buffer (2.82 KB, patch)
2010-10-29 14:37 PDT, Benoit Jacob [:bjacob] (mostly away)
vladimir: review+
vladimir: approval2.0+
Details | Diff | Splinter Review

Description User image David Humphrey (:humph) 2010-10-14 20:02:29 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2010-10-14 20:43:43 PDT
Is this a regression from 3.6?
Comment 2 User image David Humphrey (:humph) 2010-10-14 20:55:45 PDT
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 User image Vladimir Vukicevic [:vlad] [:vladv] 2010-10-15 07:43:10 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2010-10-15 08:01:23 PDT
Why does 3.6 glitch, though?
Comment 5 User image Vladimir Vukicevic [:vlad] [:vladv] 2010-10-15 11:30:00 PDT
Not sure, but I'd guess a similar type of bug..
Comment 6 User image Joe Drew (not getting mail) 2010-10-19 12:45:04 PDT
Want it fixed, but can release without the fix (considering we had it in 3.6).
Comment 7 User image Benoit Jacob [:bjacob] (mostly away) 2010-10-28 15:12:01 PDT
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 User image Benoit Jacob [:bjacob] (mostly away) 2010-10-29 14:37:48 PDT
Created attachment 487034 [details] [diff] [review]
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 User image Vladimir Vukicevic [:vlad] [:vladv] 2010-11-01 14:05:15 PDT
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 User image Benoit Jacob [:bjacob] (mostly away) 2010-11-03 13:39:59 PDT
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 User image Vladimir Vukicevic [:vlad] [:vladv] 2010-11-05 12:16:06 PDT
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 User image Benoit Jacob [:bjacob] (mostly away) 2010-11-05 13:15:53 PDT
http://hg.mozilla.org/mozilla-central/rev/3769e11d18d6

Leaving open until we're done discussing The Right Fix.
Comment 13 User image Benoit Jacob [:bjacob] (mostly away) 2011-10-07 09:56:18 PDT
Closing this; we've been using this solution for 6 months now without report of glitches AFAIK.

Note You need to log in before you can comment on or make changes to this bug.