Bug 92043 - [Chromium] WebWidget should be able to paint into a zoomed canvas without aliasing
Summary: [Chromium] WebWidget should be able to paint into a zoomed canvas without ali...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on: 94555 95253
Blocks: 91648
  Show dependency treegraph
 
Reported: 2012-07-23 16:28 PDT by Adam Barth
Modified: 2012-08-28 16:24 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.02 KB, patch)
2012-07-23 17:57 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (8.60 KB, patch)
2012-08-01 15:32 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (9.43 KB, patch)
2012-08-10 12:29 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (9.43 KB, patch)
2012-08-10 14:07 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
now with #define (9.09 KB, patch)
2012-08-21 11:00 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (9.57 KB, patch)
2012-08-22 12:25 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-07-23 16:28:05 PDT
WebWidget is missing paintOnDemandZoom, which chromium-android uses to render the link preview.
Comment 1 Adam Barth 2012-07-23 17:57:34 PDT
Created attachment 153924 [details]
Patch
Comment 2 WebKit Review Bot 2012-07-23 17:59:08 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Adam Barth 2012-07-24 14:45:21 PDT
@fishd: This is the software read back API for link preview that we discussed briefly yesterday.
Comment 4 Darin Fisher (:fishd, Google) 2012-07-26 13:07:05 PDT
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?
Comment 5 Adam Barth 2012-07-26 13:40:22 PDT
(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?
Comment 6 Tien-Ren Chen 2012-07-26 14:00:10 PDT
(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.
Comment 7 Adam Barth 2012-07-30 13:15:55 PDT
Comment on attachment 153924 [details]
Patch

I'll update this patch based on the discussion above.
Comment 8 Adam Barth 2012-08-01 15:32:05 PDT
Created attachment 155903 [details]
Patch
Comment 9 Eric Seidel (no email) 2012-08-07 16:02:39 PDT
This appears to be solving some of the same goals as bug 92275.
Comment 10 James Robinson 2012-08-10 11:32:11 PDT
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).
Comment 11 Adam Barth 2012-08-10 12:29:12 PDT
Created attachment 157790 [details]
Patch
Comment 12 WebKit Review Bot 2012-08-10 14:04:24 PDT
Comment on attachment 157790 [details]
Patch

Attachment 157790 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13472499
Comment 13 Adam Barth 2012-08-10 14:07:35 PDT
Created attachment 157807 [details]
Patch
Comment 14 James Robinson 2012-08-10 16:26:40 PDT
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)?
Comment 15 Adam Barth 2012-08-10 16:38:56 PDT
> > 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.
Comment 16 Adam Barth 2012-08-13 12:02:59 PDT
> 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.
Comment 17 Adam Barth 2012-08-20 14:36:47 PDT
@jamesr: It sounds like fishd is on vacation.  Do you still want to wait for his feedback on this patch before landing it?
Comment 18 James Robinson 2012-08-20 14:42:38 PDT
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?
Comment 19 Adam Barth 2012-08-20 14:44:37 PDT
Yes.  This one of three patches blocking API compatibility between upstream and downstream chromium-android.
Comment 20 James Robinson 2012-08-20 15:04:30 PDT
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.
Comment 21 Adam Barth 2012-08-20 15:05:18 PDT
Thanks.

@fishd: If you have comments, please don't hesitate to ask for a followup patch.
Comment 22 WebKit Review Bot 2012-08-20 16:08:04 PDT
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
Comment 23 WebKit Review Bot 2012-08-20 16:57:39 PDT
Comment on attachment 157807 [details]
Patch

Clearing flags on attachment: 157807

Committed r126095: <http://trac.webkit.org/changeset/126095>
Comment 24 WebKit Review Bot 2012-08-20 16:57:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 WebKit Review Bot 2012-08-20 17:56:21 PDT
Re-opened since this is blocked by 94555
Comment 27 Adam Barth 2012-08-21 10:24:27 PDT
Downstream change in https://chromiumcodereview.appspot.com/10828420
Comment 28 Adam Barth 2012-08-21 11:00:26 PDT
Created attachment 159724 [details]
now with #define
Comment 29 Adam Barth 2012-08-22 12:25:16 PDT
Created attachment 159989 [details]
Patch for landing
Comment 30 WebKit Review Bot 2012-08-22 13:24:33 PDT
Comment on attachment 159989 [details]
Patch for landing

Clearing flags on attachment: 159989

Committed r126344: <http://trac.webkit.org/changeset/126344>
Comment 31 WebKit Review Bot 2012-08-22 13:24:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 32 WebKit Review Bot 2012-08-28 15:09:24 PDT
Re-opened since this is blocked by 95253
Comment 33 Adam Barth 2012-08-28 15:51:07 PDT

*** This bug has been marked as a duplicate of bug 94182 ***
Comment 34 Tien-Ren Chen 2012-08-28 16:07:49 PDT
With the reworked disambiguation popup we still need the ForceSoftware flag. Chromium side CL: http://codereview.chromium.org/10885004/

Can we reopen this?
Comment 35 Adam Barth 2012-08-28 16:22:22 PDT
> Can we reopen this?

Oh!  I misunderstood.  Sorry for the confusion.
Comment 36 Adam Barth 2012-08-28 16:24:51 PDT
I'm rolling out the rollout.