Closed
Bug 1066664
Opened 11 years ago
Closed 11 years ago
[Flame] Taking screenshot in landscape mode crops images
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
People
(Reporter: njpark, Assigned: jrmuizel)
References
Details
(Keywords: regression, Whiteboard: fbimagecompare)
Attachments
(2 files, 1 obsolete file)
124.08 KB,
image/png
|
Details | |
4.46 KB,
patch
|
mattwoodrow
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•11 years ago
|
Whiteboard: fbimagecompare
Reporter | ||
Updated•11 years ago
|
status-b2g-v2.2:
--- → affected
Reporter | ||
Comment 1•11 years ago
|
||
However, on screen, the image is rotated properly. Only on the screenshot image the problem occurs.
Reporter | ||
Updated•11 years ago
|
Keywords: regression
Comment 2•11 years ago
|
||
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•11 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)
Comment 4•11 years ago
|
||
No-Jun - Was this working on master before bug 1064479 landed?
Flags: needinfo?(npark)
Reporter | ||
Comment 5•11 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•11 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
Reporter | ||
Comment 7•11 years ago
|
||
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•11 years ago
|
||
No-Jun - Can you provide a blocking triage here? Should this be nommed to block?
Flags: needinfo?(npark)
Comment 9•11 years ago
|
||
Jeff, let's take a look at this first thing next week.
Assignee: nobody → jmuizelaar
Flags: needinfo?(jmuizelaar)
Reporter | ||
Comment 10•11 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•11 years ago
|
Assignee: nobody → jmuizelaar
Comment 11•11 years ago
|
||
Just to be clear - we get the correct image when in the camera app, this is "just" the screenshot issue, right?
Comment 12•11 years ago
|
||
(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•11 years ago
|
||
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•11 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.
Assignee | ||
Comment 15•11 years ago
|
||
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)
Assignee | ||
Comment 16•11 years ago
|
||
This sort of works
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #8490366 -
Attachment is obsolete: true
Assignee | ||
Comment 18•11 years ago
|
||
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 19•11 years ago
|
||
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+
Comment 20•11 years ago
|
||
Do we have a try run for this patch?
Comment 21•11 years ago
|
||
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)
Assignee | ||
Comment 22•11 years ago
|
||
Comment 23•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Reporter | ||
Comment 24•11 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)
Comment 25•11 years ago
|
||
Please nominate this patch for Aurora approval when you get a chance :)
status-b2g-v2.1:
--- → affected
status-firefox33:
--- → wontfix
status-firefox34:
--- → affected
status-firefox35:
--- → fixed
Flags: needinfo?(jmuizelaar)
Comment 26•11 years ago
|
||
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?
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(jmuizelaar)
Updated•11 years ago
|
Attachment #8491143 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 27•11 years ago
|
||
Comment 28•11 years ago
|
||
Hopeful bustage fix:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ebbe836496d6
I'll backout otherwise.
Comment 29•11 years ago
|
||
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
Updated•11 years ago
|
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.
Description
•