1066664 - [Flame] Taking screenshot in landscape mode crops images

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[No-Jun Park [:njpark]]

Attachment: test_gfx_smoketest_orientation_zoom_flame_15.png

STR:
Open Browser app, and hold it in landscape mode
Press Power + volDown to take the screenshot.
Open gallery, and check the screenshot taken.

Expected:
the screenshot should take the full view of the screen.
Actual:
the screenshot only takes the partial view of the screen, as if the orientation change was not done in the screenshot

Version Info:
Gaia      b72909030e214175144342f7e5df7e88a2b52fd4
Gecko     https://hg.mozilla.org/mozilla-central/rev/59d4326311e0
BuildID   20140912061053
Version   35.0a1
ro.build.date   Fri Sep 12 09:21:09 EDT 2014
ro.bootloader   L1TC10011800

Suspecting the fix in Bug 1064479 might be the culprit, since this does NOT reproduce in master.

Comment 1

[No-Jun Park [:njpark]]

However, on screen, the image is rotated properly.  Only on the screenshot image the problem occurs.

Comment 2

[Jason Smith [:jsmith]]

What do you mean by this doesn't reproduce on master? Did you test this with a custom build?

Comment 3

[No-Jun Park [:njpark]]

(In reply to Jason Smith [:jsmith] from comment #2)
> What do you mean by this doesn't reproduce on master? Did you test this with
> a custom build?

Ah sorry, I meant mozilla-aurora.  Typo.

Comment 4

[Jason Smith [:jsmith]]

No-Jun - Was this working on master before bug 1064479 landed?

Comment 5

[No-Jun Park [:njpark]]

(In reply to Jason Smith [:jsmith] from comment #4)
> No-Jun - Was this working on master before bug 1064479 landed?

Yes it was working until two days ago at least.  Will flash yesterday's master and retry.

Comment 6

[No-Jun Park [:njpark]]

Last good tinderbox build:
https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/tinderbox-builds/mozilla-central-flame-kk-eng/20140911063332/
Gaia      e3b9d0d6516177636965d97c63c60981a24a0662
Gecko     https://hg.mozilla.org/mozilla-central/rev/98ea98c8191a
BuildID   20140911063332
Version   35.0a1


First bad tinderbox build:
https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/tinderbox-builds/mozilla-central-flame-kk-eng/20140911145721/

Gaia      e3b9d0d6516177636965d97c63c60981a24a0662
Gecko     https://hg.mozilla.org/mozilla-central/rev/eec8eb2b4dd6
BuildID   20140911145721
Version   35.0a1

Comment 7

[No-Jun Park [:njpark]]

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=98ea98c8191a&tochange=eec8eb2b4dd6 includes http://hg.mozilla.org/mozilla-central/rev/0ba56b8f5946, so worth investigating whether it's the cause of this bug.

Comment 8

[Jason Smith [:jsmith]]

No-Jun - Can you provide a blocking triage here? Should this be nommed to block?

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Jeff, let's take a look at this first thing next week.

Comment 10

[No-Jun Park [:njpark]]

[Blocking Requested - why for this release]:
Pre-noming the bug for 2.1.  Currently this does not happen in 2.1, but I suspect the changeset that fixes Bug 1064479 (which is a 2.1 blocker) causes it.  If that is the case, then the fix for this bug should also be uplifted to 2.1 as well.  

If the changeset for Bug 1064479 was NOT causing it, I'll change the nom to 2.2.

This bug causes the screenshot to be taken in wrong orientation in landscape mode, losing portion of the screen in the captured image.  Also regression.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Just to be clear - we get the correct image when in the camera app, this is "just" the screenshot issue, right?

Comment 12

[Tony Chung [:tchung]]

(In reply to Milan Sreckovic [:milan] from comment #11)
> Just to be clear - we get the correct image when in the camera app, this is
> "just" the screenshot issue, right?

yep, screenshotting feature causes this regression.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

I'm assuming screenshot feature is a supported feature, rather than something we just use for debugging.  With that in mind, 2.1+.  However, if doing the screenshot is something that we don't actively expose to the users and is really just a debugging feature, I think we may be OK without the fix.

Comment 14

[No-Jun Park [:njpark]]

I think this is a public feature that regular users are aware of.  It's basically same as the feature of the android kernel (Power + VolDown) that works in most android phones of 4.0+.  

Also the daily automation test I run to compare screenshot images rely heavily on this feature at the moment, since marionette supports the handling of this feature as well.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

I had a look at this today. CompositorOGL::CopyToTarget gets a draw target in landscape mode but the screen back buffer is always in portrait previously there was a world transform that would additionally applied in this function.

i.e. previously we had:
                   
landscape content (worldXform'd)-> portrait framebuffer ->(worldXfrom'd) -> landscape buffer

in bug 1064479 I removed the world Xform which means we have:

portrait content -> portrait framebuffer -> landscape

which is broken.

My feeling is that we can probably apply the screen rotation to the draw target and that should just fix things without having to plumb that world Xform back into the compositor.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Attachment: WIP

This sort of works

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Attachment: A better version

Comment 18

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8491143 [details] [diff] [review]
A better version

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

This moves the screenshot (now-nonexistent) unrotation code from the opengl compositor into ClientLayerManager.cpp

Comment 19

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8491143 [details] [diff] [review]
A better version

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

::: gfx/layers/client/ClientLayerManager.cpp
@@ +454,5 @@
>          RefPtr<DataSourceSurface> surf = GetSurfaceForDescriptor(inSnapshot);
>          DrawTarget* dt = mShadowTarget->GetDrawTarget();
>          Rect dstRect(bounds.x, bounds.y, bounds.width, bounds.height);
>          Rect srcRect(0, 0, bounds.width, bounds.height);
> +        gfx::Matrix oldMatrix = dt->GetTransform();

Maybe use AutoSaveTransform from gfx/2d/Helpers.h

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Do we have a try run for this patch?

Comment 21

[Tony Chung [:tchung]]

Hi Jeff, is there any reason this patch can't be landed now and also uplifted to 2.1 if tests go green?  Thanks

Comment 22

[Jeff Muizelaar [:jrmuizel]]

It is landed :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/baafb688f138

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/baafb688f138

Comment 24

[No-Jun Park [:njpark]]

This looks good to me, both this fix and bug 1064479 should land for 2.1, since bug 1064479 was waiting for the fix to land on master for it to land on 2.1

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Please nominate this patch for Aurora approval when you get a chance :)

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8491143 [details] [diff] [review]
A better version

Approval Request Comment
See the Aurora request comments on bug 1071241.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/167b548389d3

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Hopeful bustage fix:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ebbe836496d6

I'll backout otherwise.

Comment 29

[Roland Kunkel [:RolandK]]

Verified as fixed for the latest 2.1 and 2.2 Flame builds:

Environmental Variables:
----------------------------------------
Device: Flame 2.2 Master
BuildID: 20141011040204
Gaia: 95f580a1522ffd0f09302372b78200dab9b6f322
Gecko: 3f6a51950eb5
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Environmental Variables:
----------------------------------------
Device: Flame 2.1
BuildID: 20141011000201
Gaia: f5d4ff60ffed8961f7d0380ada9d0facfdfd56b1
Gecko: d813d79d3eae
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

When taking a screenshot in landscape mode, the resulting screenshot is properly created as such.