Closed Bug 1066664 Opened 9 years ago Closed 9 years ago

[Flame] Taking screenshot in landscape mode crops images


(Core :: Graphics: Layers, defect)

35 Branch
Gonk (Firefox OS)
Not set



blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- verified
b2g-v2.2 --- verified


(Reporter: njpark, Assigned: jrmuizel)



(Keywords: regression, Whiteboard: fbimagecompare)


(2 files, 1 obsolete file)

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

the screenshot should take the full view of the screen.
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
BuildID   20140912061053
Version   35.0a1   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.
Whiteboard: fbimagecompare
However, on screen, the image is rotated properly.  Only on the screenshot image the problem occurs.
Keywords: regression
What do you mean by this doesn't reproduce on master? Did you test this with a custom build?
Flags: needinfo?(npark)
(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)
(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)
Last good tinderbox build:
Gaia      e3b9d0d6516177636965d97c63c60981a24a0662
BuildID   20140911063332
Version   35.0a1

First bad tinderbox build:

Gaia      e3b9d0d6516177636965d97c63c60981a24a0662
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)
[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)
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+
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)
Attached patch WIP (obsolete) — Splinter Review
This sort of works
Attached patch A better versionSplinter Review
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)
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)
Closed: 9 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
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.
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.