Bug 98518 - [Qt][WK2] Plugins are completely broken with a custom device pixel ratio
Summary: [Qt][WK2] Plugins are completely broken with a custom device pixel ratio
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords:
: 94821 (view as bug list)
Depends on:
Blocks: 98528
  Show dependency treegraph
 
Reported: 2012-10-05 07:18 PDT by Balazs Kelemen
Modified: 2012-10-29 07:15 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.73 KB, patch)
2012-10-05 07:28 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2012-10-05 07:18:12 PDT
Currently this is the case in MiniBrowser because it defines 1.5 as device pixel ratio. My goal currently is just to handle this case. We will probably have to return to this if we want to support plugins not just on the desktop.
Comment 1 Balazs Kelemen 2012-10-05 07:28:10 PDT
Created attachment 167326 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-10-05 08:56:26 PDT
Comment on attachment 167326 [details]
Patch

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

> Source/WebKit2/Shared/qt/ShareableBitmapQt.cpp:91
> -    // See <https://bugs.webkit.org/show_bug.cgi?id=64663>.
> -    notImplemented();
> +    QImage image = createQImage();
> +    QPainter* painter = context.platformContext();
> +
> +    painter->save();
> +    painter->scale(scaleFactor, scaleFactor);
> +    painter->drawImage(dstPoint, image, QRect(srcRect));
> +    painter->restore();

why not do a fast path for  scaleFactor == 1.0?
Comment 3 Balazs Kelemen 2012-10-05 09:26:41 PDT
(In reply to comment #2)
> (From update of attachment 167326 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167326&action=review
> 
> > Source/WebKit2/Shared/qt/ShareableBitmapQt.cpp:91
> > -    // See <https://bugs.webkit.org/show_bug.cgi?id=64663>.
> > -    notImplemented();
> > +    QImage image = createQImage();
> > +    QPainter* painter = context.platformContext();
> > +
> > +    painter->save();
> > +    painter->scale(scaleFactor, scaleFactor);
> > +    painter->drawImage(dstPoint, image, QRect(srcRect));
> > +    painter->restore();
> 
> why not do a fast path for  scaleFactor == 1.0?

It's already there :) (above the changed lines)
Comment 4 Kenneth Rohde Christiansen 2012-10-05 09:29:54 PDT
Comment on attachment 167326 [details]
Patch

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

>>> Source/WebKit2/Shared/qt/ShareableBitmapQt.cpp:91
>>> +    painter->restore();
>> 
>> why not do a fast path for  scaleFactor == 1.0?
> 
> It's already there :) (above the changed lines)

Is it possible somehow to use the GraphicsContext3D to use the hardware for scaling? iguess you are basically scaling an image right?
Comment 5 Balazs Kelemen 2012-10-05 09:40:02 PDT
(In reply to comment #4)
> (From update of attachment 167326 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167326&action=review
> 
> >>> Source/WebKit2/Shared/qt/ShareableBitmapQt.cpp:91
> >>> +    painter->restore();
> >> 
> >> why not do a fast path for  scaleFactor == 1.0?
> > 
> > It's already there :) (above the changed lines)
> 
> Is it possible somehow to use the GraphicsContext3D to use the hardware for scaling? iguess you are basically scaling an image right?

Yep, but we do software rendering here by design. The backing store is a shared memory buffer. Probably we could utilize ShareableSurface to make this accelerated.
Comment 6 Balazs Kelemen 2012-10-05 09:51:06 PDT
Comment on attachment 167326 [details]
Patch

Clearing flags on attachment: 167326

Committed r130519: <http://trac.webkit.org/changeset/130519>
Comment 7 Balazs Kelemen 2012-10-05 09:51:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Andras Becsi 2012-10-10 07:21:23 PDT
Since r130630 applies the device pixel ratio as a UI-side scale in the desktop webview plugins seem to be scaled twice, this issue seems to be fixes if reverting this change.
Can you check, and confirm this?
Comment 9 Balazs Kelemen 2012-10-13 12:07:31 PDT
*** Bug 94821 has been marked as a duplicate of this bug. ***
Comment 10 Balazs Kelemen 2012-10-18 08:09:31 PDT
(In reply to comment #8)
> Since r130630 applies the device pixel ratio as a UI-side scale in the desktop webview plugins seem to be scaled twice, this issue seems to be fixes if reverting this change.
> Can you check, and confirm this?

First of all, if I simply revert it than we will not paint at all again, so I think you mean we should ignore the scale factor.

It seems to be more complicated. Now the plugin is overscaled. Actually now I think that my code is wrong, we should scale the image, not the context (or we can emulate scaling the image with coordinate transform on the context to be faster). However, if I change it like that it doesn't help.

If I ignore the scaleFactor at all, it's better but still not perfect. I'm not sure about what is wrong in this case, but the buttons at the bottom of the video don't show up because the video covers that area as well and there are glitches in the video (maybe it's a missing clip or smg).

If I change PluginProxy::contentScaleFactor to return constant 1, than it's shown correctly. We should find out what's the difference between us and mac. Actually by reading the code it seems to me that the scale is really applied twice in PluginProxy, so I will take a look at it on Mac.
Comment 11 Balazs Kelemen 2012-10-18 08:14:54 PDT
Filed bug 9722 for the overscaling.
Comment 12 Jocelyn Turcotte 2012-10-18 08:28:14 PDT
Comment on attachment 167326 [details]
Patch

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

> Source/WebKit2/Shared/qt/ShareableBitmapQt.cpp:89
> +    painter->scale(scaleFactor, scaleFactor);

What if you just don't scale it here?
I don't really understand all this code, but it seems like this shareablebitmap is the backing store of the plugin view and it is already scaled by PluginProxy, no?
Also, does this mean that the plugin backing store isn't a layer by itself? Does it get re-blitted on every tile of the non-composited content for every update of the plugin?

It's unlikely that we'll have a plugin big enough to need tiling, so why not pass the shareablebitmap directly to the ui process like we do for CoordinatedGraphicsLayer::setContentsToImage?

Anyway this is just food for thought, if we can just fix the scaling issue I'd be pretty happy too.
Comment 13 Balazs Kelemen 2012-10-18 08:50:56 PDT
(In reply to comment #12)
> (From update of attachment 167326 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167326&action=review
> 
> > Source/WebKit2/Shared/qt/ShareableBitmapQt.cpp:89
> > +    painter->scale(scaleFactor, scaleFactor);
> 
> What if you just don't scale it here?

This is the case when the panel at the bottom does not shown up.

> I don't really understand all this code, but it seems like this shareablebitmap is the backing store of the plugin view and it is already scaled by PluginProxy, no?

PluginProxy has an m_pluginBackingStore, this is the shared memory the plugin process paints onto, and m_backingStore which is shared between the UI and the web process. m_pluginBackingStore is getting blitted to m_backingStore in update. For me it also seems to be that the scaleFactor is applied multiple times: first in the plugin process, than when we blit, than when we blit m_backingStore to the graphicsContext in paint. I have no clue how could this work on mac.

> Also, does this mean that the plugin backing store isn't a layer by itself? Does it get re-blitted on every tile of the non-composited content for every update of the plugin?

I guess this is the case with the tiles covering the plugin but I'm not sure.

> 
> It's unlikely that we'll have a plugin big enough to need tiling, so why not pass the shareablebitmap directly to the ui process like we do for CoordinatedGraphicsLayer::setContentsToImage?

Sounds good but I need to investigate more.
Comment 14 Jocelyn Turcotte 2012-10-19 01:45:29 PDT
(In reply to comment #13)
> I have no clue how could this work on mac.

It's possible that they're now always using CA layers in their case (just a guess).
Comment 15 Balazs Kelemen 2012-10-29 07:15:01 PDT
(In reply to comment #11)
> Filed bug 9722 for the overscaling.

it's bug 99722