New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 865430 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 9
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

DevTools for Android Doesn't Work With Viz

Project Member Reported by jonr...@chromium.org, Jul 19

Issue description

1) PageHandler

Supports remote debugging by casting screenshots of the page on Android to the hosting devtools on desktop.

A while ago we added support for DevTools remote debugging in Viz. This is in PageHandler::OnFrameFromVideoConsumer

However at the time Viz on Android wasn't far enough along for this to become the system-wide default. So all of this is not compiled in Android.

2) DevToolsEyeDropper

This was updated to make use of viz::ClientFrameSinkVideoCapturer however the legacy path is still around.

This leaves DevTools as the last consumer of WebContentsObserver::DidReceiveCompositorFrame.

samans@ you worked on DevToolsEyeDropper, and in general on Viz Video Capture. Is this something that is ready for use on Android?

 
Labels: -Pri-3 M-69 Pri-1
Status: Assigned (was: Untriaged)
Hmm I could’ve sworn I’ve used Devtools with Viz recently. Maybe it was just surface sync. I’m marking this as P1. 
I was just trying this with yesterdays ToT as I wanted to delete the legacy listener.

A few extra tests:

1) PageHandler does work on Android if I remove the compilation guards. But I would like to hear if samans@ has any concerns with the capture before landing it.

2) Eye Dropper does not seem to work when remote debugging an Android Device in Viz.
Cc: kylec...@chromium.org ericrk@chromium.org
+kylechar@, +ericrk@
Status: Started (was: Assigned)
OK good news. Eye dropper doesn't even work without viz so no fix needed there. Performance timeline already works with viz to my surprise. Only screen-casting doesn't work which seems to be easily fixable by removing compile guards for android.
jonross: By the way let's not remove the old code path until M68 is shipped to stable with the new code path.
We also seem to miss top controls height and ratio but they can be easily added to VideoFrameMetadata.
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b2e7b3e635af6d9fcf7928d30f6887c9d3fae692

commit b2e7b3e635af6d9fcf7928d30f6887c9d3fae692
Author: Saman Sami <samans@chromium.org>
Date: Sat Jul 21 00:48:09 2018

Fix screencasting on Android with Viz enabled

Use video capture for screencasting. Also ship top controls height and
shown ratio.

Bug:  865430 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I6cc7b945fb2937a6552c33ca8b5be10b566a48e1
Reviewed-on: https://chromium-review.googlesource.com/1145231
Reviewed-by: Saman Sami <samans@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577048}
[modify] https://crrev.com/b2e7b3e635af6d9fcf7928d30f6887c9d3fae692/components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.cc
[modify] https://crrev.com/b2e7b3e635af6d9fcf7928d30f6887c9d3fae692/content/browser/devtools/protocol/page_handler.cc
[modify] https://crrev.com/b2e7b3e635af6d9fcf7928d30f6887c9d3fae692/content/browser/devtools/protocol/page_handler.h
[modify] https://crrev.com/b2e7b3e635af6d9fcf7928d30f6887c9d3fae692/media/base/video_frame_metadata.h

Status: Fixed (was: Started)
Status: Assigned (was: Fixed)
The patch in #8 fixes screencasting.

However it doesn't touch on the eyedropper.
samans@ noted that it appears that the eyedropper doesn't work for remoting to Android even without Viz. Which seems bad since it can be accessed easily.

I'm not sure if if worked before any of our work in the devtools area. Did we want to continue to track that portion here, or should we generate a new issue to forward to devtools owners?
I believe that should be a separate bug. Our main goal is to reach performance parity with non-viz, not to introduce new features in DevTools.

I don't think the eyedropper is easily accessible actually. We have two choices: either inspect the pixels of the video frames (which have low quality), or somehow ship higher quality images from the remote device, which doesn't sound like a trivial task if one is not familiar with DevTools.
Status: Fixed (was: Assigned)
Labels: Merge-Request-69 Target-69
Status: Available (was: Fixed)
Seems like when Viz is disabled we receive snapshots both from CopyFromSurface and the video capturer. I'm gonna have to fix this and also verify that screencasting still works on Android WebView.
Project Member

Comment 14 by sheriffbot@chromium.org, Jul 28

Labels: -Merge-Request-69 Hotlist-Merge-Approved Merge-Approved-69
Your change meets the bar and is auto-approved for M69. Please go ahead and merge the CL to branch 3497 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 31

Labels: -merge-approved-69 merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4e57b6ba91b30a73648619653c94a6768e015cd7

commit 4e57b6ba91b30a73648619653c94a6768e015cd7
Author: Saman Sami <samans@chromium.org>
Date: Tue Jul 31 15:42:31 2018

Fix screencasting on Android with Viz enabled

Use video capture for screencasting. Also ship top controls height and
shown ratio.

Bug:  865430 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I6cc7b945fb2937a6552c33ca8b5be10b566a48e1
Reviewed-on: https://chromium-review.googlesource.com/1145231
Reviewed-by: Saman Sami <samans@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#577048}(cherry picked from commit b2e7b3e635af6d9fcf7928d30f6887c9d3fae692)
Reviewed-on: https://chromium-review.googlesource.com/1156565
Cr-Commit-Position: refs/branch-heads/3497@{#272}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/4e57b6ba91b30a73648619653c94a6768e015cd7/components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.cc
[modify] https://crrev.com/4e57b6ba91b30a73648619653c94a6768e015cd7/content/browser/devtools/protocol/page_handler.cc
[modify] https://crrev.com/4e57b6ba91b30a73648619653c94a6768e015cd7/content/browser/devtools/protocol/page_handler.h
[modify] https://crrev.com/4e57b6ba91b30a73648619653c94a6768e015cd7/media/base/video_frame_metadata.h

Project Member

Comment 16 by bugdroid1@chromium.org, Jul 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f623d0050021d59066c8cc9a828f5b51dddec522

commit f623d0050021d59066c8cc9a828f5b51dddec522
Author: Saman Sami <samans@chromium.org>
Date: Tue Jul 31 17:54:58 2018

Don't call CopyFromSurface when using video capture for screencasting

Early-out in OnSwapCompositorFrame if video_consumer_ exists. Don't do
this in OnSynchronousSwapCompositorFrame because video capture doesn't
work with Android WebView and we still have to call CopyFromSurface.

Bug:  865430 
Change-Id: I4b59848b2c288fc06708187b8d48e86c1fd19237
Reviewed-on: https://chromium-review.googlesource.com/1156871
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579473}
[modify] https://crrev.com/f623d0050021d59066c8cc9a828f5b51dddec522/content/browser/devtools/protocol/page_handler.cc

Labels: Merge-Request-69
Requesting to merge #16 into M69. This is a simple change that prevents DevTools from capturing every frame twice.
Project Member

Comment 18 by bugdroid1@chromium.org, Aug 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/93b0efcef471fa5de61ce8d478b7928d9cc96cdc

commit 93b0efcef471fa5de61ce8d478b7928d9cc96cdc
Author: Saman Sami <samans@chromium.org>
Date: Wed Aug 01 14:45:36 2018

Don't instantiate video_consumer_ in PageHandler on Android WebView

Video capture doesn't work on Android WebView, so don't instantiate
video_consumer_ in PageHandler

Bug:  813929 ,  865430 
Change-Id: I09ecce34f1f2a50d23fc12e0907a0d27bf3bc9d8
Reviewed-on: https://chromium-review.googlesource.com/1157206
Commit-Queue: Saman Sami <samans@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579798}
[modify] https://crrev.com/93b0efcef471fa5de61ce8d478b7928d9cc96cdc/content/browser/devtools/protocol/page_handler.cc

Project Member

Comment 19 by sheriffbot@chromium.org, Aug 1

Labels: -Merge-Request-69 Merge-Approved-69
Your change meets the bar and is auto-approved for M69. Please go ahead and merge the CL to branch 3497 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 20 by bugdroid1@chromium.org, Aug 1

Labels: -merge-approved-69
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a33caf38e8281a1f616669c3d41c1bc106597962

commit a33caf38e8281a1f616669c3d41c1bc106597962
Author: Saman Sami <samans@chromium.org>
Date: Wed Aug 01 18:50:54 2018

Don't call CopyFromSurface when using video capture for screencasting

Early-out in OnSwapCompositorFrame if video_consumer_ exists. Don't do
this in OnSynchronousSwapCompositorFrame because video capture doesn't
work with Android WebView and we still have to call CopyFromSurface.

Bug:  865430 
Change-Id: I4b59848b2c288fc06708187b8d48e86c1fd19237
Reviewed-on: https://chromium-review.googlesource.com/1156871
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#579473}(cherry picked from commit f623d0050021d59066c8cc9a828f5b51dddec522)
Reviewed-on: https://chromium-review.googlesource.com/1159141
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#317}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/a33caf38e8281a1f616669c3d41c1bc106597962/content/browser/devtools/protocol/page_handler.cc

Status: Assigned (was: Available)
Project Member

Comment 22 by bugdroid1@chromium.org, Aug 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cd7b545a89f8bf643883709bcd05ce1fb5aedd94

commit cd7b545a89f8bf643883709bcd05ce1fb5aedd94
Author: Saman Sami <samans@chromium.org>
Date: Sat Aug 04 17:14:27 2018

Fix performance timeline screenshots on Android WebView

They just don't work at all right now. Video capture doesn't work on
Android WebView. We need to call CopyFromSurface.

Bug:  813929 ,  865430 
Change-Id: I4360d3f53f852eb7e3182ac58540a1500cc6fee3
Reviewed-on: https://chromium-review.googlesource.com/1159199
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580759}
[modify] https://crrev.com/cd7b545a89f8bf643883709bcd05ce1fb5aedd94/content/browser/devtools/protocol/tracing_handler.cc
[modify] https://crrev.com/cd7b545a89f8bf643883709bcd05ce1fb5aedd94/content/browser/devtools/render_frame_devtools_agent_host.cc

Labels: Merge-Request-69
Requesting to merge #22 into M69. This unbreaks an important feature in DevTools that currently doesn't work at all.
Project Member

Comment 24 by sheriffbot@chromium.org, Aug 4

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-69 Merge-Approved-69
Merge approved for M69 branch 3497.
Project Member

Comment 26 by bugdroid1@chromium.org, Aug 8

Labels: -merge-approved-69
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18d95c0176dcd4b9c835328c6c175fc854bb282b

commit 18d95c0176dcd4b9c835328c6c175fc854bb282b
Author: Saman Sami <samans@chromium.org>
Date: Wed Aug 08 18:11:11 2018

Fix performance timeline screenshots on Android WebView

They just don't work at all right now. Video capture doesn't work on
Android WebView. We need to call CopyFromSurface.

Bug:  813929 ,  865430 
Change-Id: I4360d3f53f852eb7e3182ac58540a1500cc6fee3
Reviewed-on: https://chromium-review.googlesource.com/1159199
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#580759}(cherry picked from commit cd7b545a89f8bf643883709bcd05ce1fb5aedd94)
Reviewed-on: https://chromium-review.googlesource.com/1167763
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#503}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/18d95c0176dcd4b9c835328c6c175fc854bb282b/content/browser/devtools/protocol/tracing_handler.cc
[modify] https://crrev.com/18d95c0176dcd4b9c835328c6c175fc854bb282b/content/browser/devtools/render_frame_devtools_agent_host.cc

Status: Fixed (was: Assigned)

Sign in to add a comment