Closed Bug 863922 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: Graphics: Layers, defect, major)

All
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox22 --- unaffected
firefox23 + fixed

People

(Reporter: fryn, Assigned: mattwoodrow)

References

Details

(Keywords: regression, verifyme)

Attachments

(1 file, 3 obsolete files)

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
Attached patch Clear content scale factor (obsolete) — Splinter Review
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)
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.
Attached patch Clear content scale factor v2 (obsolete) — Splinter Review
Attachment #739926 - Attachment is obsolete: true
Attachment #739926 - Flags: review?(bgirard)
Attachment #739930 - Flags: review?(bgirard)
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
Comment on attachment 739930 [details] [diff] [review]
Clear content scale factor v2

This doesn't seem to fix the bug for me.
Attached patch Clear content scale factor v3 (obsolete) — Splinter Review
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
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.
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 #740024 - Flags: review?(bgirard)
Attachment #740513 - 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+
Assignee: nobody → matt.woodrow
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Duplicate of this bug: 866144
Duplicate of this bug: 866210
Are there specific plugins and/or live content where this was visible so QA can verify it's fixed?
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.
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.