Bug 87094 – GeneratorGeneratedImage should cache images for the non-tiled case

• Status: REOPENED
• Component: Layout and Rendering

[Jin Yang]

no pattern draw should also be cached.

[Jin Yang]

Created attachment 143226 [details]
Patch

[Tim Horton]

Comment on attachment 143226 [details]
Patch

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

The reason I didn't immediately do this before was because I wasn't sure what the overhead of the adding the intermediate image buffer would be; have you tested that?

> Source/WebCore/ChangeLog:8
> +        No new tests.Performance optimization. 

Space after the first period please.

[Jin Yang]

Hi Tim,

I was running the IE benchmark http://ie.microsoft.com/testdrive/Graphics/Bayou/
and found the cache problem of generated image. When I try to start the patch for caching the image in the draw function, I found your patch is just committed. Your patch has cached the image in drawPattern while the path in draw still can't be cached. So based on your change, I cached the image for draw. With this patch, the benchmark boost from 34 to 52FPS with 50 firefiles. Please help review this, thanks!

Jin

[Tim Horton]

(In reply to comment #3)
> Hi Tim,
> 
> I was running the IE benchmark http://ie.microsoft.com/testdrive/Graphics/Bayou/
> and found the cache problem of generated image. When I try to start the patch for caching the image in the draw function, I found your patch is just committed. Your patch has cached the image in drawPattern while the path in draw still can't be cached. So based on your change, I cached the image for draw. With this patch, the benchmark boost from 34 to 52FPS with 50 firefiles. Please help review this, thanks!
> 
> Jin

I don't have review, so I can't help you too much. The change looks fine otherwise.

I'm glad to see it helps a benchmark, that's excellent!

[Jin Yang]

Created attachment 143234 [details]
Patch

[Jin Yang]

(In reply to comment #4)
> (In reply to comment #3)
> > Hi Tim,
> > 
> > I was running the IE benchmark http://ie.microsoft.com/testdrive/Graphics/Bayou/
> > and found the cache problem of generated image. When I try to start the patch for caching the image in the draw function, I found your patch is just committed. Your patch has cached the image in drawPattern while the path in draw still can't be cached. So based on your change, I cached the image for draw. With this patch, the benchmark boost from 34 to 52FPS with 50 firefiles. Please help review this, thanks!
> > 
> > Jin
> 
> I don't have review, so I can't help you too much. The change looks fine otherwise.
> 
> I'm glad to see it helps a benchmark, that's excellent!

Yes, thanks for your former great work, the Bayou benchmarks boost 50% with litter code change!

[WebKit Review Bot]

Comment on attachment 143234 [details]
Patch

Attachment 143234 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12748376

New failing tests:
fast/gradients/css3-linear-angle-gradients.html
fast/gradients/border-image-gradient-sides-and-corners.html
fast/borders/border-image-scaled-gradient.html
fast/gradients/css3-radial-gradients2.html
fast/gradients/css3-linear-right-angle-gradients.html
fast/gradients/css3-repeating-linear-gradients.html
fast/gradients/css3-radial-gradients.html
fast/gradients/css3-repeating-radial-gradients.html
fast/css/transformed-mask.html

[WebKit Review Bot]

Created attachment 143251 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Tim Horton]

(In reply to comment #7)
> (From update of attachment 143234 [details])
> Attachment 143234 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12748376
> 
> New failing tests:
> fast/gradients/css3-linear-angle-gradients.html
> fast/gradients/border-image-gradient-sides-and-corners.html
> fast/borders/border-image-scaled-gradient.html
> fast/gradients/css3-radial-gradients2.html
> fast/gradients/css3-linear-right-angle-gradients.html
> fast/gradients/css3-repeating-linear-gradients.html
> fast/gradients/css3-radial-gradients.html
> fast/gradients/css3-repeating-radial-gradients.html
> fast/css/transformed-mask.html

Jin: these will need new baselines.

[Simon Fraser (smfr)]

Comment on attachment 143234 [details]
Patch

Patch looks OK but please investigate the EWS failures.

[Tim Horton]

(In reply to comment #10)
> (From update of attachment 143234 [details])
> Patch looks OK but please investigate the EWS failures.

They're all super minor single-digit color deltas.

Conveniently, I think this patch will also fix https://bugs.webkit.org/show_bug.cgi?id=19942 in a somewhat roundabout way.

[Jin Yang]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 143234 [details] [details])
> > Patch looks OK but please investigate the EWS failures.
> 
> They're all super minor single-digit color deltas.
> 
> Conveniently, I think this patch will also fix https://bugs.webkit.org/show_bug.cgi?id=19942 in a somewhat roundabout way.

I have checked the result, just as you said, they are all super minor single-digit color deltas. I think that's because the former approach draws the gradient on the fly and our patch caches the gradient image first and then tiles the image to destination, this way may lose little pixel precision.

I am not familiar with the rebaseline process. Could you suggest the guide to do rebaseline? Thanks

[Jin Yang]

Created attachment 143718 [details]
Patch

[Jin Yang]

Created attachment 143752 [details]
Patch

[Jin Yang]

HiSimon,

I have updated the patch with adding the test expectations. The rebaseline should after the patch is in. Could you please review the patch? Thanks!
Jin

[Simon Fraser (smfr)]

Comment on attachment 143752 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests.Performance optimization.

Missing space. You need some explanation of the change in the changlog here.

> Source/WebCore/platform/graphics/GeneratorGeneratedImage.cpp:55
> +    // Tile the image buffer to the destination

This is the untiled case, so this comment is inaccurate.

[Jin Yang]

Created attachment 144796 [details]
Patch

[Jin Yang]

updated the patch according to simon's comments

[Simon Fraser (smfr)]

Comment on attachment 144796 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        GeneratorGeneratedImage should cache images for no pattern draw

"for no pattern draw" is confusing here. It should be "for the non-tiled drawing case" or something.

> Source/WebCore/ChangeLog:9
> +        By compared to generate image on the fill, it will loss litter pixel precision

I'm not sure what "loss litter" means. Do you mean "lose a little"?

You should mention the performance gains.

[Jin Yang]

Created attachment 145020 [details]
Patch

[Jin Yang]

(In reply to comment #19)
> (From update of attachment 144796 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144796&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        GeneratorGeneratedImage should cache images for no pattern draw
> 
> "for no pattern draw" is confusing here. It should be "for the non-tiled drawing case" or something.
> 
> > Source/WebCore/ChangeLog:9
> > +        By compared to generate image on the fill, it will loss litter pixel precision
> 
> I'm not sure what "loss litter" means. Do you mean "lose a little"?
> 
> You should mention the performance gains.

Thanks for the comments. Corrected the typo and description.

[Simon Fraser (smfr)]

Comment on attachment 145020 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests needed. We cache the generated image if generator is not changed.

The "No new tests needed" should be on a separate line after the description of the change.

> Source/WebCore/ChangeLog:10
> +        and several layout tests should be rebaselined.With this patch, the IE test drive

Space after .

[Jin Yang]

Created attachment 145245 [details]
Patch

[Jin Yang]

(In reply to comment #22)
> (From update of attachment 145020 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145020&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests needed. We cache the generated image if generator is not changed.
> 
> The "No new tests needed" should be on a separate line after the description of the change.
> 
> > Source/WebCore/ChangeLog:10
> > +        and several layout tests should be rebaselined.With this patch, the IE test drive
> 
> Space after .

updated the patch.

[Simon Fraser (smfr)]

Comment on attachment 145245 [details]
Patch

Note that related code is being cleaned up in bug 88063.

[Jin Yang]

Created attachment 145970 [details]
Patch

[Jin Yang]

(In reply to comment #26)
> Created an attachment (id=145970) [details]
> Patch

uploaded the patch, and also did clean up.

[Jin Yang]

Created attachment 146177 [details]
Patch

[Jin Yang]

seems test_expectation need to rebase, so uploaded the rebased patch.

[WebKit Review Bot]

Comment on attachment 146177 [details]
Patch

Attachment 146177 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12911249

New failing tests:
fast/reflections/reflection-with-zoom.html

[WebKit Review Bot]

Created attachment 146268 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Jin Yang]

Created attachment 146490 [details]
Patch

[Jin Yang]

(In reply to comment #32)
> Created an attachment (id=146490) [details]
> Patch

Hi Simon,

The updated patch is ready, please help review it. Thanks

[Jin Yang]

Hi Simon,

It seems the patch was not committed. I added the commit-queue? flag.

Jin

[Simon Fraser (smfr)]

Comment on attachment 146490 [details]
Patch

You should set cq? when you request review.

[WebKit Review Bot]

Comment on attachment 146490 [details]
Patch

Clearing flags on attachment: 146490

Committed r120033: <http://trac.webkit.org/changeset/120033>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 91454

[Dana Jansens]

Created attachment 152656 [details]
LayoutTest

here's a LayoutTest to demonstrate the CSS background gradient breakage.