Bug 102824 – [Skia] Add missing OpaqueRegionSkia notifier calls

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Florin Malita]

https://bugs.webkit.org/show_bug.cgi?id=102694#c3

Per comments, didDraw... calls should be added to all PlatformContextSkia's draw wrappers.

[Dana Jansens]

If there are any places that were currently calling didDraw, and now will no longer in 102694, then we could see regressions with artifacts appearing on screen. So, if that's the case this really shouldn't be done as a separate after-step. If not, then we should be alright.

[Florin Malita]

(In reply to comment #1)
> If there are any places that were currently calling didDraw, and now will no longer in 102694, then we could see regressions with artifacts appearing on screen. So, if that's the case this really shouldn't be done as a separate after-step. If not, then we should be alright.

102694 should not produce any functional changes: it just relocates existing didDraw calls for drawOval(), drawPath(), drawPoints() & drawRect() into the corresponding PlatformContextSkia wrappers. For the cases you mentioned, no didDraw calls were in place previously.

I take it it's still a good idea to add these though?

[Dana Jansens]

Yes absolutely!

[Florin Malita]

I've talked with Alok about this - being able to compute a bounding rects for positioned text in particular, to avoid doing an unbounded invalidation.

Currently there is no good API for that purpose, so it would take some work on the Skia side to get it in place. On the other hand, Alok mentioned he's hacking in this area and his patches would either

a) provide a better way to get bounding info from the WK text-handling methods (which already have that).

or

b) eliminate the need for opaque region notifications at this level altogether by refactoring the subsystem.

I'll let Alok share the details if needed, but the agreement was to wait for a bit to see what comes out of his work before pushing too hard on this.

Does that sound like a good plan, or would you prefer we try an interim fix? Maybe just add the notifiers for non-text related draw calls as a start?

[Dana Jansens]

I think we should add calls for correctness, or new callers of these methods may cause really bad rendering artifacts (compositor thinks something is opaque but it is not). If we don't have good data for text, we should didDrawUnbounded with a FIXME to do better.

[Florin Malita]

(In reply to comment #5)
> I think we should add calls for correctness, or new callers of these methods may cause really bad rendering artifacts (compositor thinks something is opaque but it is not). If we don't have good data for text, we should didDrawUnbounded with a FIXME to do better.

Alright, will do.

[Florin Malita]

Created attachment 176371 [details]
Patch

[Dana Jansens]

Comment on attachment 176371 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:387
> +        m_opaqueRegion.didDrawRect(this, dst, *paint, &bitmap);

If dst is larger than the bitmap, does it get stretched or does it only fill the top/left?

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:850
> +    EXPECT_EQ_RECT(IntRect(10, 10, 90, 90), platformContext.opaqueRegion().asRect());

Since the opaque region isn't changing, it's not clear that the tracking did anything at all here.

There seems to be a number of cases like that. Splitting this up into more tests and/or doing a clear/drwa and verifying the opaque region/pixels in between would help.

[Florin Malita]

(In reply to comment #8)
> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:387
> > +        m_opaqueRegion.didDrawRect(this, dst, *paint, &bitmap);
> 
> If dst is larger than the bitmap, does it get stretched or does it only fill the top/left?

It gets scaled/resampled.

> > Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:850
> > +    EXPECT_EQ_RECT(IntRect(10, 10, 90, 90), platformContext.opaqueRegion().asRect());
> 
> Since the opaque region isn't changing, it's not clear that the tracking did anything at all here.


The intent was to guard against false positives. But I guess that path is thoroughly covered in the other unit tests - I'll clean them up.

[Florin Malita]

Created attachment 176474 [details]
Patch

Updated per comments.

[Dana Jansens]

Comment on attachment 176474 [details]
Patch

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

Thanks! A couple comments on beefing up the tests, and then I'm good with it!

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:749
> +    platformContext.drawPosTextH("A", 1, &pointX, 0, alphaPaint);

add cases with an opaquePaint, that shows it doesn't mark anything opaque, but doesn't destroy opaque areas?

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:807
> +    platformContext.writePixels(alphaBitmap, 10, 50);

Checking (10, 0) = no change and (10, 1) = drops the left edge, might be better here, to show that the height of the bitmap is being respected.

Similar for (0, 10) and (1, 10).

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:857
> +    platformContext.drawBitmap(alphaBitmap, 50, 10, &paint);

similar as writePixels, this doesn't really demonstrate that the bitmap's width/height is being used correctly yet.

> Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp:903
> +    platformContext.drawBitmapRect(alphaBitmap, 0, SkRect::MakeXYWH(10, 10, 50, 10), &paint);

Same idea again, but maybe (10, 0, 10, 10), (10, 0, 10, 11), (0, 10, 10, 10), and (0, 10, 11, 10).

[Florin Malita]

Created attachment 176566 [details]
Patch

[Florin Malita]

(In reply to comment #11)
> Thanks! A couple comments on beefing up the tests, and then I'm good with it!

Thanks, updated.

[Dana Jansens]

Comment on attachment 176566 [details]
Patch

Thanks :) LGTM

[Stephen White]

Comment on attachment 176566 [details]
Patch

Looks good (bots willing).  r=me

[WebKit Review Bot]

Comment on attachment 176566 [details]
Patch

Clearing flags on attachment: 176566

Committed r136125: <http://trac.webkit.org/changeset/136125>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.