Bug 73186 – [Chromium]Draw small cached canvases to an onscreen large canvas is slow when 2D canvas is HW accelerated.

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Jin Yang]

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.

[Jin Yang]

Created attachment 116699 [details]
patch for this bug

[Jin Yang]

any comments for this issue?

[James Robinson]

I'm not sure I fully understand the consequences of this patch. Perhaps Stephen or Mike could take a look.

[Stephen White]

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.

[Jin Yang]

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.

[Stephen White]

(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.

[Justin Novosad]

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

[Brian Salomon]

(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.

[Jin Yang]

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

[Tommy Widenflycht]

Created attachment 126509 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Tommy Widenflycht]

Yikes! Uploaded my patch to the wrong bug... So sorry!

[Brian Salomon]

Can this bug be closed?

[Jin Yang]

It has been fixed in 
https://bugs.webkit.org/show_bug.cgi?id=68420