Closed Bug 1066664 Opened 6 years ago Closed 6 years ago
[Flame] Taking screenshot in landscape mode crops images
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.
However, on screen, the image is rotated properly. Only on the screenshot image the problem occurs.
What do you mean by this doesn't reproduce on master? Did you test this with a custom build?
(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.
No-Jun - Was this working on master before bug 1064479 landed?
(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.
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
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.
No-Jun - Can you provide a blocking triage here? Should this be nommed to block?
Jeff, let's take a look at this first thing next week.
Assignee: nobody → jmuizelaar
[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.
Assignee: jmuizelaar → nobody
blocking-b2g: --- → 2.1?
Just to be clear - we get the correct image when in the camera app, this is "just" the screenshot issue, right?
(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.
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.
blocking-b2g: 2.1? → 2.1+
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.
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.
This sort of works
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
Attachment #8491143 - Flags: review?(matt.woodrow)
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
Attachment #8491143 - Flags: review?(matt.woodrow) → review+
Do we have a try run for this patch?
Hi Jeff, is there any reason this patch can't be landed now and also uplifted to 2.1 if tests go green? Thanks
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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
Please nominate this patch for Aurora approval when you get a chance :)
Comment on attachment 8491143 [details] [diff] [review] A better version Approval Request Comment See the Aurora request comments on bug 1071241.
Attachment #8491143 - Flags: approval-mozilla-aurora?
Attachment #8491143 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hopeful bustage fix: https://hg.mozilla.org/releases/mozilla-aurora/rev/ebbe836496d6 I'll backout otherwise.
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.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
You need to log in before you can comment on or make changes to this bug.