869178 - [Skia] Use drawBitmapRect instead of creating a bitmap shader when drawing images in DrawTargetSkia

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[George Wright (:gw280) (needinfo me!)]

drawBitmap* methods on SkCanvas support slicing up bitmaps larger than the maximum texture size when using the GL backend for Skia, but using bitmap shaders does not. We should therefore use drawBitmapRect.

Comment 1

[George Wright (:gw280) (needinfo me!)]

Attachment: Use drawBitmapRect

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 746059 [details] [diff] [review]
Use drawBitmapRect

Review of attachment 746059 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! Does this fix some of the drawImage() failures I've seen on some things?

Comment 3

[George Wright (:gw280) (needinfo me!)]

It should fix any instance of the "failed to create texture for bitmap" type error.

Comment 4

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/mozilla-central/rev/f9a17848125a

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Backed out for reftest crashes. Thanks for watching your push after landing on m-c, btw.
https://hg.mozilla.org/mozilla-central/rev/b25afb305360

https://tbpl.mozilla.org/php/getParsedLog.php?id=22711192&tree=Mozilla-Central

Comment 6

[Ryan VanderMeulen [:RyanVM]]

And a mochitest failure.
https://tbpl.mozilla.org/php/getParsedLog.php?id=22711902&tree=Mozilla-Central

And a reftest unexpected pass.
https://tbpl.mozilla.org/php/getParsedLog.php?id=22711772&tree=Mozilla-Central

Comment 7

[George Wright (:gw280) (needinfo me!)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> Backed out for reftest crashes. Thanks for watching your push after landing
> on m-c, btw.

I was watching, just got back a little bit too late. Very weird, because the patches passed try without issue... Hrmmmmm...

Comment 8

[Andreas Gal :gal]

    // The reason we do clipping is because Skia doesn't support SkRect as                                                                                                        
    // source rect. See http://crbug.com/145540.                                                                                                                                  
    // When Skia supports then use this as the source rect to replace 0.                                                                                                          
    //                                                                                                                                                                            
    // scaledSrcRect.offset(-enclosingScaledSrcRect.x(), -enclosingScaledSrcRect.y());                                                                                            
    context->save();
    context->clipRect(destRectVisibleSubset);

    // Because the image fragment is generated with an approxmiated scaling                                                                                                       
    // factor. This draw will perform a close to 1 scaling.                                                                                                                       
    //                                                                                                                                                                            
    // NOTE: For future optimization. If the difference in scale is so small                                                                                                      
    // that Skia doesn't produce a difference then we can just blit it directly                                                                                                   
    // to enhance performance.                                                                                                                                                    
    context->drawBitmapRect(scaledImageFragment, 0, enclosingDestRect, &paint);
    context->restore();

I think you are failing the tests because you just truncate the source rect. We should do what chrome does here.

Comment 9

[George Wright (:gw280) (needinfo me!)]

The current version of the patch that is on the graphics branch does what Chrome does.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5cc97a9ea30