Don't apply scaling twice to plugin instances in HiDPI mode

RESOLVED FIXED in Firefox 23

Status

()

Core
Graphics: Layers
--
major
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: fryn, Assigned: mattwoodrow)

Tracking

({regression, verifyme})

Trunk
mozilla23
All
Mac OS X
regression, verifyme
Points:
---

Firefox Tracking Flags

(firefox22 unaffected, firefox23+ fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Plugin objects are incorrectly being drawn at twice their size and getting clipped in HiDPI mode.

I identified that the following changeset caused this regression:
https://hg.mozilla.org/mozilla-central/rev/61c218d62004
(Assignee)

Comment 1

5 years ago
Created attachment 739926 [details] [diff] [review]
Clear content scale factor

This is what the old code (MacIOSurfaceImage) used to do.

It effectively clears the mContentsScaleFactor set on the IOSurface since we apply scaling through nsObjectFrame/ImageLayer already.
Attachment #739926 - Flags: review?(bgirard)
(Reporter)

Comment 2

5 years ago
Comment on attachment 739926 [details] [diff] [review]
Clear content scale factor

I get the following error when building with this:

gfx/gl/GLContextProviderCGL.mm:463:12: error: cannot convert 'mozilla::TemporaryRef<MacIOSurface>' to 'SharedTextureHandle' (aka 'unsigned long') without a conversion operator
return (SharedTextureHandle)MacIOSurface::LookupSurface(surf->GetIOSurfaceID());
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
(Assignee)

Comment 3

5 years ago
Created attachment 739930 [details] [diff] [review]
Clear content scale factor v2
Attachment #739926 - Attachment is obsolete: true
Attachment #739926 - Flags: review?(bgirard)
Attachment #739930 - Flags: review?(bgirard)
(Reporter)

Updated

5 years ago
Summary: Plugin objects are incorrectly being drawn at twice their size and getting clipped in HiDPI mode → Don't apply scaling twice to plugin instances in HiDPI mode
Duplicate of this bug: 863977
(Reporter)

Comment 5

5 years ago
Comment on attachment 739930 [details] [diff] [review]
Clear content scale factor v2

This doesn't seem to fix the bug for me.
(Assignee)

Comment 6

5 years ago
Created attachment 740024 [details] [diff] [review]
Clear content scale factor v3

Alright, I set up fake-HiDPI mode and actually tested it this time.

This one definitely compiles and fixes the bug for me :)
Attachment #740024 - Flags: review?(bgirard)
Duplicate of this bug: 864093

Updated

5 years ago
Attachment #739930 - Attachment is obsolete: true
Attachment #739930 - Flags: review?(bgirard)
Comment on attachment 740024 [details] [diff] [review]
Clear content scale factor v3

Review of attachment 740024 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ +752,5 @@
>          NS_ASSERTION(image->GetFormat() == SHARED_TEXTURE, "Wrong format?");
>  
>          SharedTextureImage::Data data;
>          data.mShareType = GLContext::SameProcess;
> +        RefPtr<MacIOSurface> newSurf = MacIOSurface::LookupSurface(ioSurface->GetIOSurfaceID());

Is the lookupsurface a hard requirement here? This function shows up in profiles.
(Assignee)

Comment 9

5 years ago
Created attachment 740513 [details] [diff] [review]
Clear content scale factor v4

The previous patch was just matching the old behaviour, but sure, lets make it better.

I added the GetDevicePixel**() getters, since it's not obvious from outside the class that you need to take the ceil of GetContentsScaleFactor(). Plus it seems like a waste to have to divide and then re-multiply by the same thing.
Attachment #740024 - Attachment is obsolete: true
Attachment #740513 - Flags: review?(bgirard)
Attachment #740024 - Flags: review?(bgirard)
Comment on attachment 740513 [details] [diff] [review]
Clear content scale factor v4

Review of attachment 740513 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with a buildable patch

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ +758,5 @@
>                                                                  GLContext::IOSurface);
>          data.mInverted = false;
> +        // Use the device pixel size of the IOSurface, since layers handles resolution scaling
> +        // already.
> +        data.mSize = gfxIntSize(newSurf->GetDevicePixelWidth(), newSurf->GetDevicePixelHeight());

newSurf? Does it build?
Attachment #740513 - Flags: review?(bgirard) → review+

Updated

5 years ago
Assignee: nobody → matt.woodrow
status-firefox22: --- → unaffected
tracking-firefox23: --- → +
(Reporter)

Comment 11

5 years ago
Comment on attachment 740513 [details] [diff] [review]
Clear content scale factor v4

Review of attachment 740513 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ +758,5 @@
>                                                                  GLContext::IOSurface);
>          data.mInverted = false;
> +        // Use the device pixel size of the IOSurface, since layers handles resolution scaling
> +        // already.
> +        data.mSize = gfxIntSize(newSurf->GetDevicePixelWidth(), newSurf->GetDevicePixelHeight());

s/newSurf/ioSurface/g makes this build and fixes the bug.

Thanks for working on this and reviewing this, Matt & Benoit. :)
Duplicate of this bug: 865371
Duplicate of this bug: 865782
Duplicate of this bug: 865937
https://hg.mozilla.org/mozilla-central/rev/972fffa53784
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox23: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Duplicate of this bug: 866144

Updated

5 years ago
Duplicate of this bug: 866210
Are there specific plugins and/or live content where this was visible so QA can verify it's fixed?
Keywords: verifyme

Comment 20

5 years ago
Go to united.com and search for a flight. You should see a flash animation of a rotating plane. If thats bigger than the center of the screen, you have the bug. If not, you are ok.
This reappeared in yesterday's (?) Nightly.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sounds like bug 907792.
(Reporter)

Updated

4 years ago
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.