Closed Bug 780920 Opened 7 years ago Closed 7 years ago

Cannot take on-device screenshot for Apps running in OOP (calculator, browser, Calendar)

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: tchung, Assigned: cjones)

References

Details

Attachments

(6 files, 2 obsolete files)

9.81 KB, image/png
Details
2.60 KB, patch
roc
: review+
Details | Diff | Splinter Review
17.37 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.92 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.61 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.27 KB, patch
Details | Diff | Splinter Review
I am unable to do an on-device screenshot for apps that are running Out Of Process.   take the screenshot, and when you open it, its just a blank white screen.

Apps affected:  Calendar, Browser, Calculator

Repro:
1) install otoro daily: 8-7-2012
2) launch an OOP app (Calendar, Browser, or Calculator)
3) hold down HOME and Power button, 2 vibrates.
4) screenshot notification appears
5) open the screenshot in a image viewer, and verify the image is showing blank white content.

Expected:
- screenshotting OOP apps should still show the content that it took

Actual:
- screenshot shows white blank screen
Summary: Cannot take a on device screenshot for Apps running in OOP (calculator, browser, Calendar) → Cannot take on-device screenshot for Apps running in OOP (calculator, browser, Calendar)
Are we doing a drawWindow() on the top-level chrome window with USE_WIDGET_LAYERS?  I tried to warn about this when we discussed screenshotting way back when ...
Yes, we're doing exactly that.  See b2g/chrome/content/shell.js:606

Note, though, that the screenshots are probably just a side-effect of something worse.  The more florid symptom is that in b2g-desktop (using yesterday's build) calculator and calendar display blank white screens.  Browser displays chrome, but the content area is always blank white.

Maybe this bug needs a different subject line?
I can reproduce the screenshot bug on otoro-gb running the most recent nightly.
Seems plausible.  I don't see white screens on those apps with latest inbound on nexus s.  If we're failing to load apps OOP that would explain this bug.
CrystalSkull defaults to oop.  I've confirmed that screenshots are blank for it, and I get a blank white screen when I launch it on the desktop.

When I modified it to launch in process, I was able to take a screenshot on the otoro, and it displayed correctly on desktop.
What happens when you screenshot it on device when running OOP?
Reread #5: screenshots work on the otoro when crystal skull is in process and they don't work when it is oop.
I did, several times, which is why I asked explicitly :).

Interesting.  That's obviously not expected.  Same results with other OOP apps, like Tasks?
Every oop app I've tried has has the behavior, but I haven't tried them all.  I think tchung's intent with the subject line for this bug was to imply that it affected all oop apps.
On b2g desktop (on MacOS) all these OOP apps display plain blank screens.  But if you press and hold the Home button to bring up the task switcher, the screen is properly rendered in the screenshot view.

So I think this bug is related to https://bugzilla.mozilla.org/show_bug.cgi?id=781000
basecamp-'ing based on the fact that it appears to only affect WebGL.  Re-nom if that's not correct :)
blocking-basecamp: ? → -
Renominating because https://bugzilla.mozilla.org/show_bug.cgi?id=781000 is blocking, and I suspect this is a related bug.

Also, see comment #2.  All of the OOP apps that don't screenshot on otoro don't display *at all* in b2g-desktop.  This is a serious WTF bug. We can't just say "webgl only"

Finally, if we don't ship a phone with working webgl, all the app writers who want to create games for our device are screwed.  Has a decision actually been made to not block for webgl?
blocking-basecamp: - → ?
I've filed a separate bug for the b2g-desktop piece of this: https://bugzilla.mozilla.org/show_bug.cgi?id=782411
*b2g-desktop-mac, unfortunately.
I also observe the same behaviour on the otoro - snapshot of an OOP app shows up as all white, but works fine for non-OOP apps.
We wouldn't hold the release for this, so not a blocker. But we should try to get this fixed sooner rather than later as to help QA.
Assignee: nobody → jones.chris.g
blocking-basecamp: ? → -
This is more than just WebGL content - this is any content on the browser (just confirmed it's any content on the browser).

This is probably a blocker for dogfooding, doing compat analysis, etc. It's also impairing our ability to give "good" bugs - so I'm inclined to renom to reconsider this as a blocker for the dogfooding issue.
blocking-basecamp: - → ?
We were aware of that during triage. It still didn't feel like we'd hold the release for this, which is the definition of a blocker (screenshots didn't seem like a required feature). But rather something we want to fix ASAP to help during development, which is why we gave it an assignment rather than blocker status.
blocking-basecamp: ? → -
This blocks reftests.
We're screenshotting from here

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayersParent.cpp#484

which doesn't go through CompositorParent and doesn't apply async scroll offsets, take position:fixed layers into account, etc.  I thought that was fixed for fennec, but apparently not :|.
Not to beat a dead horse, but rotation is another one of things not taken into account.
Component: General → Graphics: Layers
Product: Boot2Gecko → Core
Comment on attachment 663190 [details] [diff] [review]
Sketch of suggested fix for drawWindow()

Er yeah, not quite ready to land yet ;).
Attachment #663190 - Flags: review?(roc) → feedback?(roc)
roc suggests moving this logic into the layer manager, which sounds good; everything above there will continue to Just Work.  We'll move the new logic here into

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicLayerManager.cpp#1093

which already sets up state in the way we need (mShadowTarget), but doesn't ask the right thing to render to the shadow target after the txn.
Comment on attachment 663190 [details] [diff] [review]
Sketch of suggested fix for drawWindow()

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

Waiting for next patch
Attachment #663190 - Flags: feedback?(roc)
I have a working patch, but it turns out that LayerManagerOGL doesn't account for the world transform when reading back, so rotation is still broken.
flagging one more time at Triage drivers for blocking-basecamp?, given the working patches that cjones has just submitted.
blocking-basecamp: - → ?
Comment on attachment 663286 [details] [diff] [review]
part 2: Let clients query a widget's 'real' compositor, when the widget doesn't draw directly to its OS window

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

::: widget/xpwidgets/nsBaseWidget.cpp
@@ +925,5 @@
> +{
> +  if (!mCompositorChild) {
> +    return nullptr;
> +  }
> +  return nsRefPtr<CompositorChild>(mCompositorChild).forget();

Why not just return a raw CompositorChild*?

Also, rev nsIWidget IID
Comment on attachment 663287 [details] [diff] [review]
part 3: The ultimate compositor has to draw to the readback surface; shadow layers doesn't have access to all the needed rendering state

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

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +1132,5 @@
> +          AutoOpenSurface opener(OPEN_READ_ONLY, snapshot);
> +          gfxASurface* source = opener.Get();
> +
> +          gfxContextAutoSaveRestore restore(mShadowTarget);
> +          mShadowTarget->SetOperator(gfxContext::OPERATOR_SOURCE);

I don't think we should use the SOURCE operator here. OVER is correct.
Attachment #663287 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> Comment on attachment 663286 [details] [diff] [review]
> part 2: Let clients query a widget's 'real' compositor, when the widget
> doesn't draw directly to its OS window
> 
> Review of attachment 663286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/xpwidgets/nsBaseWidget.cpp
> @@ +925,5 @@
> > +{
> > +  if (!mCompositorChild) {
> > +    return nullptr;
> > +  }
> > +  return nsRefPtr<CompositorChild>(mCompositorChild).forget();
> 
> Why not just return a raw CompositorChild*?
> 

I can do that.

> Also, rev nsIWidget IID

Oh right.  I keep dreaming that I finished that de-COM patch ...
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
> Comment on attachment 663287 [details] [diff] [review]
> part 3: The ultimate compositor has to draw to the readback surface; shadow
> layers doesn't have access to all the needed rendering state
> 
> Review of attachment 663287 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/basic/BasicLayerManager.cpp
> @@ +1132,5 @@
> > +          AutoOpenSurface opener(OPEN_READ_ONLY, snapshot);
> > +          gfxASurface* source = opener.Get();
> > +
> > +          gfxContextAutoSaveRestore restore(mShadowTarget);
> > +          mShadowTarget->SetOperator(gfxContext::OPERATOR_SOURCE);
> 
> I don't think we should use the SOURCE operator here. OVER is correct.

OK, fixed.
Clearing the basecamp nom - its gone through triage a few times and has been determined to not block ship, mainly since having screenshots broken won't stop ship.
blocking-basecamp: ? → ---
I don't really care if this is blocking+ or -; it prevents us from running meaningful reftests.  As I recall, at some points in the past, that's been marked blocking+, and at other times blocking-.  Whatever.
There are some disturbing failures on tryserver with segfaults accessing the size tag of shmem segments (AFAICT) that should already be allocated, mapped, and committed.  Yay.
Repushed to see if the android fairies have fixed things in the meantime

https://tbpl.mozilla.org/?tree=Try&rev=455ea74b1a57
The gods were not kind to me.
After the fixes to reftest-remote, the harness still wouldn't launch, because this function was throwing and aborting the tests.  No idea what's going on here.  This patch could get me in trouble with Johnson & Johnson, so feel free to f-.

After spending 30 minutes trying to get various flavors of --disable-optimize builds to terminate linking, I'm taking a break to go land some other code :|.
Attachment #667331 - Flags: feedback?(mark.finkle)
(good news, though, is that I can still repro what looks like the same crash locally.)
Duplicate of this bug: 796460
Gave up on trying to get any more information out of android.  I reproduced the crash x11/omtc though.  A debug build would have told me what the problem was in about 5 seconds :(.
Chris - Do we still need the bandaid fix?
I wasn't able to run reftest-remote locally without it.
Attachment #667331 - Flags: feedback?(mark.finkle)
You need to log in before you can comment on or make changes to this bug.