Open Bug 846421 Opened 7 years ago Updated 2 years ago

mozbrowser screenshot API doesn't capture nested remote mozbrower content

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set

Tracking

()

tracking-b2g backlog
Tracking Status
b2g-v2.0 --- affected

People

(Reporter: cjones, Assigned: chiajung)

References

Details

(Whiteboard: permafail [nested-oop])

Attachments

(1 file)

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.
Duplicate of this bug: 998187
is this planned to be fixed? 
This impact URL sharing over NFC, which is 2.0 key feature
Blocks: b2g-NFC-2.0
Flags: needinfo?(fabrice)
feature-b2g: --- → 2.0
(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?
Flags: needinfo?(fabrice)
NI? to Kanru.
Flags: needinfo?(kchen)
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..
Flags: needinfo?(kchen)
This is for bug 998187.
let me know if you have further question.
Anyone else can look into this?
Flags: needinfo?(kchen)
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.
Flags: needinfo?(kchen)
Blocks: 1002336
Blocks: 980768
Hi CJ, Ivan,
This seems like a graphic issue.
Flags: needinfo?(itsay)
Flags: needinfo?(cku)
remove feature-b2g and nominate 2.0?
This is an obvious defect to new feature (URL sharing thru NFC)
blocking-b2g: --- → 2.0?
feature-b2g: 2.0 → ---
Talk to Peter and he will take a look this one from graphic side and see how this one can be proceeded.
Flags: needinfo?(itsay) → needinfo?(pchang)
Yes, | peter
Flags: needinfo?(cku)
(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.
Flags: needinfo?(pchang)
Assignee: nobody → pchang
Blocks: 931733
Plus this bug in 2.0 to fix the vendor dependent hard code in pixel format.
blocking-b2g: 2.0? → 2.0+
Per comment 10, over to Core::Graphics.
Component: General → Graphics
Whiteboard: [2.0-flame-test-run-1]
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.
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.
(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
(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
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.
blocking-b2g: 2.0+ → 2.0?
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.
(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?
Flags: needinfo?(whuang)
ni? Juwei as she takes charge of NFC UX.
Let's talk face to face.
Flags: needinfo?(whuang) → needinfo?(jhuang)
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.
blocking-b2g: 2.0? → 2.1?
Flags: needinfo?(jhuang)
No longer blocks: 931733
Nominate to 2.1 feature.
blocking-b2g: 2.1? → ---
feature-b2g: --- → 2.1
Whiteboard: [2.0-flame-test-run-1] → [2.0-flame-test-run-1][nested-oop]
QA Whiteboard: [COM=NFC]
Assignee: pchang → chung
The priority of this item has been lowered and moved to backlog of graphic work for Firefox OS. Adjust the flags accordingly.
blocking-b2g: --- → backlog
feature-b2g: 2.1 → ---
Whiteboard: [2.0-flame-test-run-1][nested-oop] → [2.0-flame-test-run-1] [2.0-flame-test-run-2] [nested-oop]
Whiteboard: [2.0-flame-test-run-1] [2.0-flame-test-run-2] [nested-oop] → permafail [nested-oop]
Attached patch WIPSplinter Review
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.
QA Whiteboard: [COM=NFC]
add tracking flag for backlog.
tracking-b2g: --- → +
blocking-b2g: backlog → ---
[Tracking Requested - why for this release]: correct as backlog.
Blocks: TV_FxOS2.5
No longer blocks: TV_FxOS2.5
You need to log in before you can comment on or make changes to this bug.