Bug 92043 – [Chromium] WebWidget should be able to paint into a zoomed canvas without aliasing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Adam Barth]

WebWidget is missing paintOnDemandZoom, which chromium-android uses to render the link preview.

[Adam Barth]

Created attachment 153924 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

@fishd: This is the software read back API for link preview that we discussed briefly yesterday.

[Darin Fisher (:fishd, Google)]

Comment on attachment 153924 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1651
> +static PageWidgetDelegate::CanvasBackground canvasBackgroundForTransparencey(bool isTransparent)

nit: avoid interleaving non-class methods with class methods.  move to the top of the file.
interleaved non-class methods tend to attract more non-class methods, which then seems to
lead to a messy .cpp file.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1681
> +    FrameView* view = page()->mainFrame()->view();

It is not obvious why this is called paintOnDemandZoom.  I was expecting
to see some code here that triggered a temporary zoom of the content.
As is, it just seems like the paint() method except that it forces
software rendering and compositing.  It seems like WebViewImpl::paint()
might produce exactly the same result.  In the accelerated case, it'll
just do the work on the GPU and then read back the pixels.

Do we really need this new paint function?

[Adam Barth]

(In reply to comment #4)
> (From update of attachment 153924 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153924&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:1681
> > +    FrameView* view = page()->mainFrame()->view();
> 
> It is not obvious why this is called paintOnDemandZoom.  I was expecting
> to see some code here that triggered a temporary zoom of the content.
> As is, it just seems like the paint() method except that it forces
> software rendering and compositing.  It seems like WebViewImpl::paint()
> might produce exactly the same result.  In the accelerated case, it'll
> just do the work on the GPU and then read back the pixels.

The intent is to apply the zoom by scaling the canvas that gets passed in.

> Do we really need this new paint function?

The comment above the function says this function avoids aliasing, but I wonder if that comment is still correct.

@trchen: Do you know if we still need this second painting function?

[Tien-Ren Chen]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 153924 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=153924&action=review
> > 
> > > Source/WebKit/chromium/src/WebViewImpl.cpp:1681
> > > +    FrameView* view = page()->mainFrame()->view();
> > 
> > It is not obvious why this is called paintOnDemandZoom.  I was expecting
> > to see some code here that triggered a temporary zoom of the content.
> > As is, it just seems like the paint() method except that it forces
> > software rendering and compositing.  It seems like WebViewImpl::paint()
> > might produce exactly the same result.  In the accelerated case, it'll
> > just do the work on the GPU and then read back the pixels.
> 
> The intent is to apply the zoom by scaling the canvas that gets passed in.
> 
> > Do we really need this new paint function?
> 
> The comment above the function says this function avoids aliasing, but I wonder if that comment is still correct.
> 
> @trchen: Do you know if we still need this second painting function?

Yes, this function basically works the same as WebViewImpl::paint() in pure sof tware rendering path. The purpose is to explicitly avoid reusing composted result, which is essentially resampling a low-res image.

I agree the behavior of the function is a little bit confusing in the sense that the scale matrix is actually applied by the client in advance.

As for the function name, I think paintOnDemandZoom should be appropriate since it's sole purpose is for the on-demand zoom to redraw a high-res image. And I believe the function is only a temporary solution until we get something more powerful such as SkPicture and UberCompositor.

[Adam Barth]

Comment on attachment 153924 [details]
Patch

I'll update this patch based on the discussion above.

[Adam Barth]

Created attachment 155903 [details]
Patch

[Eric Seidel (no email)]

This appears to be solving some of the same goals as bug 92275.

[James Robinson]

Comment on attachment 155903 [details]
Patch

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

We have a few different versions of "software" rendering when composited:
1.) Composite and do a readback
2.) Software render for printing - ignores 3d transforms, but tries to do a readback of GPU-resident content
3.) Software flattened rendering - ignores 3d transforms, ignores GPU-resident content

Should we try to call out the different in WebWidget.h?  The print behavior is currently down on WebFrame (I'm not sure exactly why) and it's pretty subtle.

This behavior seems like the right one for link highlighting, at least in most circumstances - there will be cases where it misses the desired rect by an epic amount (due to ignoring 3d) but I'm guessing it's good enough.

> Source/WebKit/chromium/public/WebWidget.h:97
> +        // Force the widget to rerender onto the canvas using sofware.

typo "sofware"

I think you should note that this will result in some content not showing up as expected (i.e. ignoring 3d transforms) and some content to not show up at all (video, canvas, webgl).

[Adam Barth]

Created attachment 157790 [details]
Patch

[WebKit Review Bot]

Comment on attachment 157790 [details]
Patch

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

[Adam Barth]

Created attachment 157807 [details]
Patch

[James Robinson]

Comment on attachment 157807 [details]
Patch

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

Works for me, but I understand Darin had some API feedback so I'll defer to him for the final review.

> Source/WebKit/chromium/public/WebWidget.h:102
> +        // Note: This option exists on OS(ANDROID) and will hopefully be

did you want to hide the enum value too (is that possible in an API header)?

[Adam Barth]

> > Source/WebKit/chromium/public/WebWidget.h:102
> > +        // Note: This option exists on OS(ANDROID) and will hopefully be
> 
> did you want to hide the enum value too (is that possible in an API header)?

I wanted to, but I couldn't find an example.  If there's a good way to do this, let me know and I'll be happy to use it.

[Adam Barth]

> Works for me, but I understand Darin had some API feedback so I'll defer to him for the final review.

@fishd: You mentioned you might have some feedback.  Thanks.

[Adam Barth]

@jamesr: It sounds like fishd is on vacation.  Do you still want to wait for his feedback on this patch before landing it?

[James Robinson]

I don't know what his feedback was going to be, but he said he had some.  I suppose we could land and then fix it up later if you need it to land in the next week - is it blocking anything specific?

[Adam Barth]

Yes.  This one of three patches blocking API compatibility between upstream and downstream chromium-android.

[James Robinson]

Comment on attachment 157807 [details]
Patch

OK. I think this looks reasonable and we can iterate when Darin gets back if we need to make more changes.

[Adam Barth]

Thanks.

@fishd: If you have comments, please don't hesitate to ask for a followup patch.

[WebKit Review Bot]

Comment on attachment 157807 [details]
Patch

Rejecting attachment 157807 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit/chromium/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Remove redundant TOUCH_LISTENER event type

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/13539804

[WebKit Review Bot]

Comment on attachment 157807 [details]
Patch

Clearing flags on attachment: 157807

Committed r126095: <http://trac.webkit.org/changeset/126095>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 94555

[James Robinson]

This guy broke: http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/render_widget_fullscreen_pepper.cc&exact_package=chromium&q=render_widget_fullscreen_pepper&l=72

[Adam Barth]

Downstream change in https://chromiumcodereview.appspot.com/10828420

[Adam Barth]

Created attachment 159724 [details]
now with #define

[Adam Barth]

Created attachment 159989 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 159989 [details]
Patch for landing

Clearing flags on attachment: 159989

Committed r126344: <http://trac.webkit.org/changeset/126344>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 95253

[Adam Barth]

*** This bug has been marked as a duplicate of bug 94182 ***

[Tien-Ren Chen]

With the reworked disambiguation popup we still need the ForceSoftware flag. Chromium side CL: http://codereview.chromium.org/10885004/

Can we reopen this?

[Adam Barth]

> Can we reopen this?

Oh!  I misunderstood.  Sorry for the confusion.

[Adam Barth]

I'm rolling out the rollout.