Bug 73186 - [Chromium]Draw small cached canvases to an onscreen large canvas is slow when 2D canvas is HW accelerated.
Summary: [Chromium]Draw small cached canvases to an onscreen large canvas is slow when...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Jin Yang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-27 22:29 PST by Jin Yang
Modified: 2012-02-17 00:58 PST (History)
10 users (show)

See Also:


Attachments
patch for this bug (4.24 KB, patch)
2011-11-27 22:43 PST, Jin Yang
no flags Details | Formatted Diff | Diff
Patch (10.85 KB, patch)
2012-02-10 06:15 PST, Tommy Widenflycht
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jin Yang 2011-11-27 22:29:36 PST
Run GUIMark3 "Bitmap Test" benchmark in chromium on Linux.
http://www.craftymind.com/guimark3/

This benchmark has two scenario, normal mode:GM3_JS_Bitmap.html and cached mode:GM3_JS_Bitmap_cache.html
The normal mode show 60FPS and cached mode show about 12 FPS on my platform when enable accelerated-2d-canvas. 

The normal mode: draw several small images(less than 128*128) into onscreen canvas(480*640) by call context.drawImage() when running.
The cached mode: when initialize, create a canvas for each image and draw image into this canvas. When running, just draw these cached canvases into the onscreen canvas.

The root cause is: in cached mode, when draw the cached canvas to onscreen canvas, it will load the cached canvas(ImageBuffer) into texture, but the generation id bounded with this ImageBuffer's bitmap is not saved because every time in ImageBuffer::draw(), we create a new Image based on the bitmap.
So it will not be found in the cached texture next time and will be loaded to gpu texture every time which show poor performance.

The fix is to hold an Image member for ImageBuffer, then no need to create an Image every time before draw, just like other port like ImageBufferQt.cpp does.

After the fix, the cached mode show 60FPS in my platform, same to normal mode.
Comment 1 Jin Yang 2011-11-27 22:43:04 PST
Created attachment 116699 [details]
patch for this bug
Comment 2 Jin Yang 2011-12-06 19:23:15 PST
any comments for this issue?
Comment 3 James Robinson 2011-12-07 18:03:35 PST
I'm not sure I fully understand the consequences of this patch. Perhaps Stephen or Mike could take a look.
Comment 4 Stephen White 2011-12-08 13:59:14 PST
Comment on attachment 116699 [details]
patch for this bug

View in context: https://bugs.webkit.org/attachment.cgi?id=116699&action=review

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:122
> +    m_data.m_image = BitmapImageSingleFrameSkia::create(*m_data.m_platformContext.bitmap(), false);

Thank you for the patch.

I'm wondering how this change will behave in the non-static case:  if you subsequently change the source canvas and re-draw it to the destination, since this bitmap has already been copied will it be successfully generation-bumped and the texture re-uploaded?  Have you tested this case?

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:164
> +    if (context == m_context) {

I've been changing this code a bit recently.  You'll have to sync up to tip-of-tree, since this condition has changed.
Comment 5 Jin Yang 2011-12-09 07:32:29 PST
Thanks for your remainder, I just tested the non-static case you mentioned: I randomly draw rect with different color(fillRect) into the cached canvas, the texture was not re-uploaded.
After debug, I found the root cause is as follow:
1. in fillRect to the cached canvas, it use canvas->fDevice->bitmap to finish the operation. As pixels changed, the bitmap's generation id will set to 0;
2. when draw this cached canvas to accelerated big canvas, it will use m_data.m_image->bitmap(as my fix) to finish the operation. the bitmap's generation id is not 0, so we will use this id to find the cached texture, the texture is not re-loaded.

I tried the below code, that is to check bitmap's content is changed before we draw. It works and the texture will be re-uploaded after the source canvas content changed:
	// if the bitmap's content has changed, we have to update m_image.
	if( m_data.m_platformContext.bitmap()->getGenerationID() != m_data.m_image.get()->nativeImageForCurrentFrame()->bitmap().getGenerationID()) {
		m_data.m_image = BitmapImageSingleFrameSkia::create(*m_data.m_platformContext.bitmap(), false);
	}

	if (drawNeedsCopy(m_context.get(), context)) {
		// We are drawing to our own buffer, copy the source buffer first
		RefPtr<Image> copy = copyImage(CopyBackingStore);
		context->drawImage(copy.get(), styleColorSpace, destRect, srcRect, op, useLowQualityScale);
	} else
		context->drawImage(m_data.m_image.get(), styleColorSpace, destRect, srcRect, op, useLowQualityScale);
 
Any suggestion to it? Thanks

(In reply to comment #4)
> (From update of attachment 116699 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116699&action=review
> 
> > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:122
> > +    m_data.m_image = BitmapImageSingleFrameSkia::create(*m_data.m_platformContext.bitmap(), false);
> 
> Thank you for the patch.
> 
> I'm wondering how this change will behave in the non-static case:  if you subsequently change the source canvas and re-draw it to the destination, since this bitmap has already been copied will it be successfully generation-bumped and the texture re-uploaded?  Have you tested this case?
> 
> > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:164
> > +    if (context == m_context) {
> 
> I've been changing this code a bit recently.  You'll have to sync up to tip-of-tree, since this condition has changed.
Comment 6 Stephen White 2011-12-09 07:54:25 PST
(In reply to comment #5)
> Thanks for your remainder, I just tested the non-static case you mentioned: I randomly draw rect with different color(fillRect) into the cached canvas, the texture was not re-uploaded.
> After debug, I found the root cause is as follow:
> 1. in fillRect to the cached canvas, it use canvas->fDevice->bitmap to finish the operation. As pixels changed, the bitmap's generation id will set to 0;
> 2. when draw this cached canvas to accelerated big canvas, it will use m_data.m_image->bitmap(as my fix) to finish the operation. the bitmap's generation id is not 0, so we will use this id to find the cached texture, the texture is not re-loaded.
> 
> I tried the below code, that is to check bitmap's content is changed before we draw. It works and the texture will be re-uploaded after the source canvas content changed:
>     // if the bitmap's content has changed, we have to update m_image.
>     if( m_data.m_platformContext.bitmap()->getGenerationID() != m_data.m_image.get()->nativeImageForCurrentFrame()->bitmap().getGenerationID()) 

I don't think explicitly checking the generation ID is that right way to go; this should be handled internally (although perhaps Brian can comment on that).

I'm wondering if the root cause of this issue might be related to
https://bugs.webkit.org/show_bug.cgi?id=74183.  If that issue were fixed, perhaps we could come up with a cleaner fix to this issue as well.

{
>         m_data.m_image = BitmapImageSingleFrameSkia::create(*m_data.m_platformContext.bitmap(), false);
>     }
> 
>     if (drawNeedsCopy(m_context.get(), context)) {
>         // We are drawing to our own buffer, copy the source buffer first
>         RefPtr<Image> copy = copyImage(CopyBackingStore);
>         context->drawImage(copy.get(), styleColorSpace, destRect, srcRect, op, useLowQualityScale);
>     } else
>         context->drawImage(m_data.m_image.get(), styleColorSpace, destRect, srcRect, op, useLowQualityScale);
> 
> Any suggestion to it? Thanks
> 
> (In reply to comment #4)
> > (From update of attachment 116699 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=116699&action=review
> > 
> > > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:122
> > > +    m_data.m_image = BitmapImageSingleFrameSkia::create(*m_data.m_platformContext.bitmap(), false);
> > 
> > Thank you for the patch.
> > 
> > I'm wondering how this change will behave in the non-static case:  if you subsequently change the source canvas and re-draw it to the destination, since this bitmap has already been copied will it be successfully generation-bumped and the texture re-uploaded?  Have you tested this case?
> > 
> > > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:164
> > > +    if (context == m_context) {
> > 
> > I've been changing this code a bit recently.  You'll have to sync up to tip-of-tree, since this condition has changed.
Comment 7 Justin Novosad 2011-12-09 08:03:54 PST
It is possible that a preferable fix for this issue would be to make sure that the generation ID is managed correctly.

This bug might be related: https://bugs.webkit.org/show_bug.cgi?id=74183
Comment 8 Brian Salomon 2011-12-09 08:32:18 PST
(In reply to comment #7)
> It is possible that a preferable fix for this issue would be to make sure that the generation ID is managed correctly.
> 
> This bug might be related: https://bugs.webkit.org/show_bug.cgi?id=74183

I agree. Let's not have WK directly inspecting the gen ID. It does seem likely to be the same or related to 74183.
Comment 9 Jin Yang 2011-12-11 18:31:22 PST
Thanks for the comments! Yes, I just used it to verify the root cause. I also prefer to fix it in manage the generation id correctly. I will check whether we need to do a clean fix after bug 74183
Comment 10 Tommy Widenflycht 2012-02-10 06:15:21 PST
Created attachment 126509 [details]
Patch
Comment 11 WebKit Review Bot 2012-02-10 06:17:09 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 12 Tommy Widenflycht 2012-02-10 06:22:25 PST
Yikes! Uploaded my patch to the wrong bug... So sorry!
Comment 13 Brian Salomon 2012-02-10 06:35:09 PST
Can this bug be closed?
Comment 14 Jin Yang 2012-02-17 00:58:12 PST
It has been fixed in 
https://bugs.webkit.org/show_bug.cgi?id=68420