Closed Bug 1250914 Opened 4 years ago Closed 4 years ago

Canvas video problem with SkiaGL

Categories

(Core :: Graphics, defect)

44 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: apollio82, Assigned: sotaro)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files, 4 obsolete files)

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
Component: Untriaged → Canvas: 2D
Product: Firefox → Core
Severity: normal → critical
OS: Unspecified → Android
Hardware: Unspecified → ARM
Component: Canvas: 2D → Audio/Video: Playback
Whiteboard: [gfx-noted]
Severity: critical → normal
Does sound like a bug. A/V Playback probably isn't the correct component though. Randell, where should this go?
Flags: needinfo?(rjesup)
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)
Okey, I take a look.
Assignee: nobody → sotaro.ikeda.g
Flags: needinfo?(sotaro.ikeda.g)
The problem seems to happen only when SkiaGL is used for canvas 2d. Skia without GL does not cause the problem.
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.
The problem happened on android, but it could be a potential problem on another platforms.

Bug 1284721 is a bit similar problem.
The patch addressed the problem on my android device, but it could degrade performance.
(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.
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)
Attachment #8769574 - Flags: feedback?(jmuizelaar)
Reduce calling flush than attachment 8769574 [details] [diff] [review].
Attachment #8769574 - Attachment is obsolete: true
Attachment #8770005 - Attachment is obsolete: true
Attachment #8770006 - Flags: review?(jmuizelaar)
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-
Summary: canvas video bug in android? → Canvas video problem with SkiaGL
Attachment #8770006 - Attachment is obsolete: true
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 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 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-
Apply the comment.
Attachment #8770367 - Attachment is obsolete: true
Attachment #8773608 - Flags: review?(jmuizelaar)
Attachment #8773608 - Flags: review?(jmuizelaar) → review+
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
https://hg.mozilla.org/mozilla-central/rev/f9ecbe4d60f8
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.