Closed
Bug 1250914
Opened 8 years ago
Closed 8 years ago
Canvas video problem with SkiaGL
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: apollio82, Assigned: sotaro)
Details
(Whiteboard: [gfx-noted])
Attachments
(2 files, 4 obsolete files)
268.08 KB,
image/jpeg
|
Details | |
8.96 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.97 Safari/537.36 Firefox for Android Steps to reproduce: I used Fabricjs to qickly add two video elements on canvas: <script type="text/javascript" src="/fabric.js"></script> <script type="text/javascript"> var canvas = new fabric.Canvas(document.getElementById('stage')); var videlem = document.getElementById('vid1'); var videlem2 = document.getElementById('vid2'); videlem.crossOrigin = "anonymous"; videlem.load(); videlem.play(); videlem.addEventListener("loadedmetadata", function(meta){ videlem.width = canvas.getWidth()-130; videlem.height = canvas.getHeight()-130; var video1 = new fabric.Image(videlem, { left: 50, top: 50, originX: 'center', originY: 'center' }); canvas.add(video1); if(video1.getWidth()>canvas.getWidth()) video1.scaleToWidth(canvas.getWidth()); if(video1.getHeight()>canvas.getHeight()) video1.scaleToHeight(canvas.getHeight()); //video1.center(); video1.setCoords(); canvas.setActiveObject(video1); canvas.calcOffset(); fabric.util.requestAnimFrame(function render() { canvas.renderAll(); fabric.util.requestAnimFrame(render); }); }); videlem2.crossOrigin = "anonymous"; videlem2.load(); videlem2.play(); videlem2.addEventListener("loadedmetadata", function(meta){ videlem2.width = canvas.getWidth()-130; videlem2.height = canvas.getHeight()-130; var video2 = new fabric.Image(videlem2, { left: 450, top: 300, originX: 'center', originY: 'center' }); canvas.add(video2); if(video2.getWidth()>canvas.getWidth()) video2.scaleToWidth(canvas.getWidth()); if(video2.getHeight()>canvas.getHeight()) video2.scaleToHeight(canvas.getHeight()); video2.center(); video2.setCoords(); canvas.setActiveObject(video2); canvas.calcOffset(); fabric.util.requestAnimFrame(function render() { canvas.renderAll(); fabric.util.requestAnimFrame(render); }); }); </script> Actual results: In Html5 video tags video render correctly, but when they play on canvas I see two identical streams (one video stream is overriding the other) Expected results: I expected the same behaviour of Desktop Firefox: two distinct streams like in video tags
Updated•8 years ago
|
Component: Untriaged → Canvas: 2D
Product: Firefox → Core
Severity: normal → critical
OS: Unspecified → Android
Hardware: Unspecified → ARM
Updated•8 years ago
|
Component: Canvas: 2D → Audio/Video: Playback
Whiteboard: [gfx-noted]
Comment 1•8 years ago
|
||
Does sound like a bug. A/V Playback probably isn't the correct component though. Randell, where should this go?
Flags: needinfo?(rjesup)
Comment 2•8 years ago
|
||
I seriously doubt A/V Playback here cares about whether it's android or not (and it renders correctly in desktop per report).... Best guess would be canvas (already was bounced from there, though perhaps incorrectly) or gfx code. Moving to gfx, NI sotaro as well
Component: Audio/Video: Playback → Graphics
Flags: needinfo?(rjesup) → needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 3•8 years ago
|
||
Okey, I take a look.
Assignee: nobody → sotaro.ikeda.g
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 4•8 years ago
|
||
The problem seems to happen only when SkiaGL is used for canvas 2d. Skia without GL does not cause the problem.
Assignee | ||
Comment 5•8 years ago
|
||
The problem was caused by reuse of mVideoTexture in CanvasRenderingContext2D. The same gl texture is used for different videos in CanvasRenderingContext2D. When 2nd video is rendered to CanvasRenderingContext2D, the first video rendering seemed not finished yet and 2nd video rendering override the first video rendering.
Assignee | ||
Comment 6•8 years ago
|
||
The problem happened on android, but it could be a potential problem on another platforms. Bug 1284721 is a bit similar problem.
Assignee | ||
Comment 7•8 years ago
|
||
The patch addressed the problem on my android device, but it could degrade performance.
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #6) > The problem happened on android, but it could be a potential problem on > another platforms. android uses gralloc + EGLImage for video rendering, it might affect to the problem.
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8769574 [details] [diff] [review] temporal patch - Call Flush() after video texture usage :jrmuizel, what is better to address the bug? Could the patch be acceptable workaround?
Attachment #8769574 -
Flags: feedback?(jmuizelaar)
Assignee | ||
Updated•8 years ago
|
Attachment #8769574 -
Flags: feedback?(jmuizelaar)
Assignee | ||
Comment 10•8 years ago
|
||
Reduce calling flush than attachment 8769574 [details] [diff] [review].
Attachment #8769574 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8770005 -
Attachment is obsolete: true
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=012d8be8e7b8
Assignee | ||
Updated•8 years ago
|
Attachment #8770006 -
Flags: review?(jmuizelaar)
Comment 13•8 years ago
|
||
Comment on attachment 8770006 [details] [diff] [review] patch - Call Flush() for reusing video texture Review of attachment 8770006 [details] [diff] [review]: ----------------------------------------------------------------- It seems to me like the proper way to deal with this is to give ownership of the texture id to the SourceSurface and have it responsible for deleting it when it is destroyed. That way we don't reuse the texture id for anything else.
Attachment #8770006 -
Flags: review?(jmuizelaar) → review-
Assignee | ||
Updated•8 years ago
|
Summary: canvas video bug in android? → Canvas video problem with SkiaGL
Assignee | ||
Comment 14•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8770006 -
Attachment is obsolete: true
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=da2a561ae039
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8770367 [details] [diff] [review] patch - Create gl texture for each video frame drawing to SkiaGL canvas :jrmuizel, how about this patch?
Attachment #8770367 -
Flags: review?(jmuizelaar)
Comment 17•8 years ago
|
||
Comment on attachment 8770367 [details] [diff] [review] patch - Create gl texture for each video frame drawing to SkiaGL canvas Review of attachment 8770367 [details] [diff] [review]: ----------------------------------------------------------------- I like this approach better
Comment 18•8 years ago
|
||
Comment on attachment 8770367 [details] [diff] [review] patch - Create gl texture for each video frame drawing to SkiaGL canvas Review of attachment 8770367 [details] [diff] [review]: ----------------------------------------------------------------- I think it might be better just to change the semantics of CreateSourceSurfaceFromNativeSurface to always take ownership. The only use of this function is in CanvasRenderingContext2D and it always wants to give up ownership. I'd suggest just modifying the documentation in 2D.h to mention that fact that ownership will be taken.
Attachment #8770367 -
Flags: review?(jmuizelaar) → review-
Assignee | ||
Comment 19•8 years ago
|
||
Apply the comment.
Attachment #8770367 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8773608 -
Flags: review?(jmuizelaar)
Updated•8 years ago
|
Attachment #8773608 -
Flags: review?(jmuizelaar) → review+
Comment 20•8 years ago
|
||
Pushed by sikeda@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f9ecbe4d60f8 Create gl texture for each video frame drawing to SkiaGL canvas r=jrmuizel
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f9ecbe4d60f8
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•