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

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: humph, Assigned: bjacob)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [approved-patches-landed])

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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.
Is this a regression from 3.6?
blocking2.0: --- → ?
(Reporter)

Comment 2

7 years ago
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
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: ? → -
(Assignee)

Comment 7

7 years ago
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.
(Assignee)

Comment 8

7 years ago
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.
Assignee: nobody → bjacob
Status: NEW → ASSIGNED
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+
http://hg.mozilla.org/mozilla-central/rev/3769e11d18d6

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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.