Chris Jones [:cjones] inactive; ni?/f?/r? if you need me Reporter
	Description • 12 years ago

I thought for sure this was on file somewhere else, but can't find it.

Symptom
 1. Open b2g browser app, load some page
 2. Long-press HOME to open task switcher

Notice that the content area in the browser display is white.

The problem is that the screenshot API does a drawWindow() in process.  mozbrowser embedders can't build a layer tree for remote content, so we instead just don't paint those regions.  (So they show up as white, from the default background.)

We have two ways to fix this
 A. Make mozbrowser responsible for gathering sceenshots of nested mozbrowser content.  Hacking something together that worked 80% could sorta work, but it would make a slow process even slower, and wouldn't handle lots of cases like transforms and async pan-zoom.

 B. Switch the screenshot API to requesting a capture from the compositor, for the subtree rooted at the window requesting capture.  This would be WAAAY faster, and would handle the edge cases above, but ... it also wouldn't work when the frame was hidden, since there wouldn't be a layer tree for it or its descendents.

Since we're stuck between a rock and a hard place, the best solution may be a mix.  Option (A) is probably too fragile, but we could use the current impl when frames are hidden and suffer the unpainted regions, but switch to option (B) when they're not.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 2 • 10 years ago

is this planned to be fixed? 
This impact URL sharing over NFC, which is 2.0 key feature

	[:fabrice] Fabrice Desré
	Comment 3 • 10 years ago

(In reply to Wesley Huang [:wesley_huang] from comment #2)
> is this planned to be fixed? 
> This impact URL sharing over NFC, which is 2.0 key feature

No one is assigned, and from comment 0 it looks very non trivial. Kanru, do you have time to look at this?

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 4 • 10 years ago

NI? to Kanru.

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 5 • 10 years ago

May I know why URL sharing over NFC needs to be able to take screenshots of embedded remote frame?

I think I don't have time to look at this before 2.0 FC..

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 6 • 10 years ago

This is for bug 998187.
let me know if you have further question.
Anyone else can look into this?

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 7 • 10 years ago

Just an idea, I think if we could make CanvasRenderingContext2D::DrawWindow accepts (Window or HTMLIFrameElement) then we could reuse the canvas drawing API to draw remote iframe from parent process directly.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 8 • 10 years ago

Hi CJ, Ivan,
This seems like a graphic issue.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 9 • 10 years ago

remove feature-b2g and nominate 2.0?
This is an obvious defect to new feature (URL sharing thru NFC)

	Ivan Tsay (:ITsay)
	Comment 10 • 10 years ago

Talk to Peter and he will take a look this one from graphic side and see how this one can be proceeded.

	u459114
	Comment 11 • 10 years ago

Yes, | peter

	Peter Chang[:pchang]
	Comment 12 • 10 years ago

(In reply to Kan-Ru Chen [:kanru] from comment #7)
> Just an idea, I think if we could make CanvasRenderingContext2D::DrawWindow
> accepts (Window or HTMLIFrameElement) then we could reuse the canvas drawing
> API to draw remote iframe from parent process directly.

To support the nested content screen capture, there is one option to use compositor to capture the screenshot, like the fullscreen catpure(home+power key). But this also needs to filter unnecessary components(status bar) to present the app screenshot.

	Ivan Tsay (:ITsay)
	Comment 13 • 10 years ago

Plus this bug in 2.0 to fix the vendor dependent hard code in pixel format.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 14 • 10 years ago

Per comment 10, over to Core::Graphics.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 15 • 10 years ago

We can flag it however we want to, if we're using flags to indicate priority, but there is nothing about this that's a bug fix.  We're talking about adding a new API and functionality, surely it's a feature.

	Peter Chang[:pchang]
	Comment 16 • 10 years ago

Now I have a local WIP to use compositor to capture screenshot for browser app and it works with NFC UI (show the browser screenshot).

In gecko, it requires the chrome privilege to capture screenshot by compositor because of security concern. Now I'm checking the security policy in compositor flow.

	Peter Chang[:pchang]
	Comment 17 • 10 years ago

(In reply to peter chang[:pchang][:peter] from comment #16)
> Now I have a local WIP to use compositor to capture screenshot for browser
> app and it works with NFC UI (show the browser screenshot).
> 
> In gecko, it requires the chrome privilege to capture screenshot by
> compositor because of security concern. Now I'm checking the security policy
> in compositor flow.

With above patch, I could capture correct browser app screenshot by compositor(without status bar) but also generate one frame without status bar displayed on the screen. This reason is gecko calls compositor to composite again and then read pixel back to save as screenshot.

In my patch, I hack the root frame checking[1] for browser(chrome) because browser always fails to pass this checking. After checking with frame tree, the following aFrame is browser's viewport frame. The following rootFrame is the viewport frame of status bar.

Force using widget layer by skipping this checking will cause the frame without status bar composed.

06-04 04:41:19.912  1187  1187 I Gecko   : pchang PresShell::RenderDocument checking aFrame 0xb31052b8 RootFrame 0xad07a2b8

[1]http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#4795

	Peter Chang[:pchang]
	Comment 18 • 10 years ago

(In reply to peter chang[:pchang][:peter] from comment #17)
> (In reply to peter chang[:pchang][:peter] from comment #16)
> > Now I have a local WIP to use compositor to capture screenshot for browser
> > app and it works with NFC UI (show the browser screenshot).
> > 
> > In gecko, it requires the chrome privilege to capture screenshot by
> > compositor because of security concern. Now I'm checking the security policy
> > in compositor flow.
> 
> With above patch, I could capture correct browser app screenshot by
> compositor(without status bar) but also generate one frame without status
> bar displayed on the screen. This reason is gecko calls compositor to
> composite again and then read pixel back to save as screenshot.
> 
Since I try to drawWindow with non-top root frame, it generates the new layout changes and layers tree changes in compositor side. But it supposes no need to compose the new changes to screen when generate snapshot. But there is a timing issue between these layout changes called and snapshot called. That's why I see one frame without status bar displayed.

There is one idea to add END_NO_COMPOSITE for EndTransaction to prevent the composition to screen when the layout changes, and only composite to snapshot's render target.

> In my patch, I hack the root frame checking[1] for browser(chrome) because
> browser always fails to pass this checking. After checking with frame tree,
> the following aFrame is browser's viewport frame. The following rootFrame is
> the viewport frame of status bar.
> 
> Force using widget layer by skipping this checking will cause the frame
> without status bar composed.
> 
> 06-04 04:41:19.912  1187  1187 I Gecko   : pchang PresShell::RenderDocument
> checking aFrame 0xb31052b8 RootFrame 0xad07a2b8
> 
> [1]http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.
> cpp#4795

	Jason Smith [:jsmith]
	Comment 19 • 10 years ago

This is a long-standing issue that's non-trivial to implement, so I don't think this should block nor be considered a 2.0 feature. As for the NFC concern, I think we should adjust the UX to take into account the fact that screenshots don't work in the browser.

	Peter Chang[:pchang]
	Comment 20 • 10 years ago

By drawing the non-top root frame, it causes the layout and retained layers changed.

I try to prevent the changed retained layers composition on the screen but the composition still triggers when there is new frame update from other process.

Therefore, the way to force drawing the non-top root frame by using widget layer causes several side effects.

There is another option to query the existed layer based the input 'frame' and then ask compositor to compose the sub layer tree as snapshot.

	Peter Chang[:pchang]
	Comment 21 • 10 years ago

(In reply to Jason Smith [:jsmith] from comment #19)
> This is a long-standing issue that's non-trivial to implement, so I don't
> think this should block nor be considered a 2.0 feature. As for the NFC
> concern, I think we should adjust the UX to take into account the fact that
> screenshots don't work in the browser.


wesley, I'm able to capture the browser screenshot but has other side effects in comment 20.

Any comment to fix this problem from UX side?

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 22 • 10 years ago

ni? Juwei as she takes charge of NFC UX.
Let's talk face to face.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 23 • 10 years ago

Per discussion with Peter and Juwei, let's leave with it in 2.0 considering risk of fixing at this moment.
nominate 2.1? so that we'll triage when 2.1 begins.

	Ivan Tsay (:ITsay)
	Comment 24 • 10 years ago

Nominate to 2.1 feature.

	Ivan Tsay (:ITsay)
	Comment 25 • 10 years ago

The priority of this item has been lowered and moved to backlog of graphic work for Firefox OS. Adjust the flags accordingly.

	Chiajung Hung [:chiajung]
	Comment 26 • 10 years ago

A working WIP.
The implementation now tries to preserve the semantics of DrawWindow flags, and it will always repaint all the layers since retaining fail.

it needs major clean up of debug logs, and some further investigation:
(1) WHY Layer Retaining seems fail
(2) What about security concerns 
(3) Preventing display while snapshot (if needed)
(4) Compose into SurfaceDescriptor directly rather than readback from framebuffer.

	Bobby Chien
	Comment 27 • 10 years ago

add tracking flag for backlog.

	Bobby Chien
	Comment 28 • 10 years ago

[Tracking Requested - why for this release]: correct as backlog.