[Flame] Taking screenshot in landscape mode crops images

VERIFIED FIXED in Firefox 34

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: njpark, Assigned: jrmuizel)

Tracking

({regression})

35 Branch
mozilla35
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 verified, b2g-v2.2 verified)

Details

(Whiteboard: fbimagecompare)

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
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.
Reporter

Updated

5 years ago
Whiteboard: fbimagecompare
Reporter

Updated

5 years ago
Reporter

Comment 1

5 years ago
However, on screen, the image is rotated properly.  Only on the screenshot image the problem occurs.
Reporter

Updated

5 years ago
Keywords: regression
What do you mean by this doesn't reproduce on master? Did you test this with a custom build?
Flags: needinfo?(npark)
Reporter

Comment 3

5 years ago
(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.
Flags: needinfo?(npark)
No-Jun - Was this working on master before bug 1064479 landed?
Flags: needinfo?(npark)
Reporter

Comment 5

5 years ago
(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.
Flags: needinfo?(npark)
Reporter

Comment 6

5 years ago
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
Blocks: 1064479
No-Jun - Can you provide a blocking triage here? Should this be nommed to block?
Flags: needinfo?(npark)
Jeff, let's take a look at this first thing next week.
Assignee: nobody → jmuizelaar
Flags: needinfo?(jmuizelaar)
Reporter

Comment 10

5 years ago
[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?
Flags: needinfo?(npark)
Reporter

Updated

5 years ago
Assignee: nobody → jmuizelaar
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+
Reporter

Comment 14

5 years ago
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.
Blocks: 1016539
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.
Flags: needinfo?(jmuizelaar)
Posted patch WIP (obsolete) — Splinter Review
This sort of works
Attachment #8490366 - Attachment is obsolete: true
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)
Reporter

Updated

5 years ago
See Also: → 1071171
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
Flags: needinfo?(bugzmuiz)
https://hg.mozilla.org/mozilla-central/rev/baafb688f138
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Reporter

Comment 24

5 years ago
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
Flags: needinfo?(bugzmuiz)
Please nominate this patch for Aurora approval when you get a chance :)
Flags: needinfo?(jmuizelaar)
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?
Flags: needinfo?(jmuizelaar)
Keywords: verifyme
Attachment #8491143 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.