CDP: Make colors in Page.screencast API accurate [chromium/src : master]
Patrick Brosset (Gerrit)
Peter Marshall (Gerrit)
Attention is currently required from: Patrick Brosset.
Patch set 2:Code-Review +1
2 comments:
Patchset:
LGTM for content/browser/devtools! thanks
File content/browser/devtools/protocol/page_handler.cc:
Patch Set #2, Line 214:
video_consumer_Do we need to update other users of DevToolsVideoConsumer?
To view, visit change 2539740. To unsubscribe, or for help writing mail filters, visit settings.
Patrick Brosset (Gerrit)
Attention is currently required from: Peter Marshall.
1 comment:
File content/browser/devtools/protocol/page_handler.cc:
Patch Set #2, Line 214:
video_consumer_Do we need to update other users of DevToolsVideoConsumer?
The only other user is the TracingHandler (used here https://chromedevtools.github.io/devtools-protocol/tot/Tracing/).
I *think* this user doesn't require the same level of color accuracy than the screencast user.
So, I'd prefer to leave it untouched for now.
To view, visit change 2539740. To unsubscribe, or for help writing mail filters, visit settings.
Chrome Cunningham (Gerrit)
Attention is currently required from: Dan Sanders, Peter Marshall, Patrick Brosset.
Chrome Cunningham would like Dan Sanders to review this change authored by Patrick Brosset.
CDP: Make colors in Page.screencast API accurate
When capturing frames using the Page.screencast CDP API, the colors are
a little off.
It's hard to see with the naked eye, but using a color picker, you can
see a difference between colors in those frames and colors in similar
images taken with, e.g., the snip tool on windows.
The reason for this is that the DevTools Video Consumer used by CDP
does not specify any pixel format or color space when creating its
underlying FrameSinkVideoConsumer, and therefore the default values are
used: PIXEL_FORMAT_I420, COLOR_SPACE_HD_REC709
With this fix, we make it possible for the Page.screencast API to
specify the ARGB pixel format and REC709 color space, and this seems to
produce images with accurate colors.
Bug: 1141314
Change-Id: I736963837b1c9701a660ef1af97bb071faf55488
---
M content/browser/devtools/devtools_video_consumer.cc
M content/browser/devtools/devtools_video_consumer.h
M content/browser/devtools/protocol/page_handler.cc
M media/renderers/paint_canvas_video_renderer.cc
4 files changed, 42 insertions(+), 2 deletions(-)
Chrome Cunningham (Gerrit)
Attention is currently required from: Dan Sanders, Peter Marshall, Patrick Brosset.
2 comments:
Patchset:
swapping self for Dan; more familiar with this code
File media/renderers/paint_canvas_video_renderer.cc:
Patch Set #2, Line 919:
video_frame.get()If null is possible this check should come first in the if()
To view, visit change 2539740. To unsubscribe, or for help writing mail filters, visit settings.
Dan Sanders (Gerrit)
Attention is currently required from: Peter Marshall, Patrick Brosset.
3 comments:
Patchset:
I'm not sure I understand this change
File content/browser/devtools/protocol/page_handler.cc:
Patch Set #2, Line 214:
video_consumer_->SetFormat(media::PIXEL_FORMAT_ARGB,I don't understand why the format and color space are injected here, is there some reason to expect PageHandler to know more about the desired capture parameters than the consumer?
What is the expectation if the capture is HDR or has been color corrected for a specific display?
File media/renderers/paint_canvas_video_renderer.cc:
Patch Set #2, Line 919:
PIXEL_FORMAT_ARGBI was surprised to see that mappable ARGB isn't supported by Paint(), I guess that goes to show how rare that is in media.
It would be better to just add support for ARGB in the existing code path though, as otherwise it is a burden to keep them in sync (eg. when changing the cropping, scaling, and rotation logic). It's only the last step where drawImage() is called that cares about the pixel format.
It doesn't make sense to UpdateLastImage() in this case, but if there is a zero-copy ARGB implementation in VideoImageGenerator then it shouldn't be a problem at least. Otherwise just clear the cache and handle it later.
To view, visit change 2539740. To unsubscribe, or for help writing mail filters, visit settings.
Dan Sanders (Gerrit)
Attention is currently required from: Peter Marshall, Patrick Brosset.
1 comment:
Patchset:
I'm not sure I understand this change
Sorry, this comment was part of a draft. I did not intend to publish it.
To view, visit change 2539740. To unsubscribe, or for help writing mail filters, visit settings.
Patrick Brosset (Gerrit)
Attention is currently required from: Dan Sanders, Peter Marshall.
2 comments:
File content/browser/devtools/protocol/page_handler.cc:
Patch Set #2, Line 214:
video_consumer_->SetFormat(media::PIXEL_FORMAT_ARGB,
The reason I injected the format and color here is that DevToolsVideoConsumer is used by more things that just the PageHandler and not all users of it have the same needs.
As far as I can tell, there are 2 users: TracingHandler and PageHandler.
- TracingHandler is used by the Performance tab in Chrome DevTools, presumably to display the little images in performance profiles.
- PageHandler is used by CDP clients to capture screencasts of the browser.
The goal here is to ensure the those screencasts are color correct. TracingHandler, on the other hand, probably doesn't have this requirement.
What is the expectation if the capture is HDR or has been color corrected for a specific display?
I don't know how to answer this question, I don't have enough specialized knowledge here.
File media/renderers/paint_canvas_video_renderer.cc:
Patch Set #2, Line 919:
PIXEL_FORMAT_ARGB
I was surprised to see that mappable ARGB isn't supported by Paint(), I guess that goes to show how […]
I think I lack proper knowledge here to fully understand what's required to do.
So I decided to file this issue to make Paint support ARGB: https://bugs.chromium.org/p/chromium/issues/detail?id=1153674
This way, perhaps someone who knows this code can take it from here and add support, and then I can make use of it from the DevTools side.
To view, visit change 2539740. To unsubscribe, or for help writing mail filters, visit settings.
Dan Sanders (Gerrit)
Attention is currently required from: Peter Marshall, Patrick Brosset.
3 comments:
Patchset:
Due to limitations in the PaintCanvasVideoRenderer changes I will not approve this CL as it currently is. Are there any short-term options for moving forward that don't require that change?
File content/browser/devtools/protocol/page_handler.cc:
Patch Set #2, Line 214:
video_consumer_->SetFormat(media::PIXEL_FORMAT_ARGB,
The reason I injected the format and color here is that DevToolsVideoConsumer is used by more things […]
I don't know the situation for HDR or color-corrected rendering myself, I'll assume this code is acceptable for now.
If the goal is color-correctness though it should be tested on devices running in spaces other than sRGB.
File media/renderers/paint_canvas_video_renderer.cc:
Patch Set #2, Line 919:
PIXEL_FORMAT_ARGB
I think I lack proper knowledge here to fully understand what's required to do. […]
Sounds fair. I likely won't be able to take on the ARGB change myself this quarter, but there are others (GPU experts) that are qualified.
To view, visit change 2539740. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Dan Sanders, Peter Marshall, Patrick Brosset.
1 comment:
File media/renderers/paint_canvas_video_renderer.cc:
Patch Set #2, Line 919:
PIXEL_FORMAT_ARGB
Sounds fair. […]
I think you just need to add this code to ConvertVideoFrameToRGBPixelsTask(). Since that function mostly YUV specific I'd just add this to l.389:
if (format == PIXEL_FORMAT_ARGB) {
DCHECK_LE(width, row_bytes);
const uint8_t* data = plane_meta[VideoFrame::kARGBPlane].data;
for (size_t i = chunk_start; i < chunk_start + rows; ++i) {
memcpy(pixels, data, width);
pixels += row_bytes;
data += plane_meta[VideoFrame::kARGBPlane].stride;
}
return;
}
To view, visit change 2539740. To unsubscribe, or for help writing mail filters, visit settings.
Patrick Brosset (Gerrit)
Patrick Brosset abandoned this change.
To view, visit change 2539740. To unsubscribe, or for help writing mail filters, visit settings.
Patrick Brosset (Gerrit)
Attention is currently required from: Dale Curtis.
Patrick Brosset would like Dale Curtis to review this change.
CDP: Make colors in Page.screencast API accurate
When capturing frames using the Page.screencast CDP API, the colors are
a little off.
It's hard to see with the naked eye, but using a color picker, you can
see a difference between colors in those frames and colors in similar
images taken with, e.g., the snip tool on windows.
The reason for this is that the DevTools Video Consumer used by CDP
does not specify any pixel format or color space when creating its
underlying FrameSinkVideoConsumer, and therefore the default values are
used: PIXEL_FORMAT_I420, COLOR_SPACE_HD_REC709
With this fix, we make it possible for the Page.screencast API to
specify the ARGB pixel format and REC709 color space, and this seems to
produce images with accurate colors.
Bug: 1141314
Change-Id: If06f5419c1feaebb3134a5d422769d8123f5518b
---
M content/browser/devtools/devtools_video_consumer.cc
M content/browser/devtools/devtools_video_consumer.h
M content/browser/devtools/protocol/page_handler.cc
M media/renderers/paint_canvas_video_renderer.cc
4 files changed, 28 insertions(+), 8 deletions(-)
To view, visit change 2593354. To unsubscribe, or for help writing mail filters, visit settings.
Patrick Brosset (Gerrit)
Attention is currently required from: Dale Curtis.
1 comment:
Patchset:
This is the new version of https://chromium-review.googlesource.com/c/chromium/src/+/2593354 based on your comment. Is this what you had in mind?
To view, visit change 2593354. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Patrick Brosset.
Patrick Brosset has uploaded this change for review.
CDP: Make colors in Page.screencast API accurate
When capturing frames using the Page.screencast CDP API, the colors are
a little off.
It's hard to see with the naked eye, but using a color picker, you can
see a difference between colors in those frames and colors in similar
images taken with, e.g., the snip tool on windows.
The reason for this is that the DevTools Video Consumer used by CDP
does not specify any pixel format or color space when creating its
underlying FrameSinkVideoConsumer, and therefore the default values are
used: PIXEL_FORMAT_I420, COLOR_SPACE_HD_REC709
With this fix, we make it possible for the Page.screencast API to
specify the ARGB pixel format and REC709 color space, and this seems to
produce images with accurate colors.
Bug: 1141314
Change-Id: If06f5419c1feaebb3134a5d422769d8123f5518b
---
M content/browser/devtools/devtools_video_consumer.cc
M content/browser/devtools/devtools_video_consumer.h
M content/browser/devtools/protocol/page_handler.cc
M media/renderers/paint_canvas_video_renderer.cc
4 files changed, 28 insertions(+), 8 deletions(-)
To view, visit change 2593354. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Patrick Brosset.
Patch set 1:Code-Review +1
2 comments:
Patchset:
lgtm % not relaxing the restriction for when we paint black frames so much.
File media/renderers/paint_canvas_video_renderer.cc:
Patch Set #1, Line 928:
if (!video_frame.get() || video_frame->natural_size().IsEmpty()) {Probably we only want to relax this to include the ARGB format.
To view, visit change 2593354. To unsubscribe, or for help writing mail filters, visit settings.
Andrey Kosyakov (Gerrit)
1 comment:
File media/renderers/paint_canvas_video_renderer.cc:
Patch Set #1, Line 392:
for (size_t i = chunk_start; i < chunk_start + rows; ++i) {s/chunk_start/0/?
To view, visit change 2593354. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Andrey Kosyakov, Patrick Brosset.
1 comment:
File media/renderers/paint_canvas_video_renderer.cc:
Patch Set #1, Line 392:
for (size_t i = chunk_start; i < chunk_start + rows; ++i) {s/chunk_start/0/?
chunk_start may not be zero, so I don't think that's right. This is a sharded task, so each thread only copies some of the rows.
To view, visit change 2593354. To unsubscribe, or for help writing mail filters, visit settings.
Patrick Brosset (Gerrit)
Attention is currently required from: Dale Curtis, Andrey Kosyakov.
2 comments:
File media/renderers/paint_canvas_video_renderer.cc:
Patch Set #1, Line 392:
for (size_t i = chunk_start; i < chunk_start + rows; ++i) {
chunk_start may not be zero, so I don't think that's right. […]
I don't think this is related to this because I'm seeing all of the rows in the resulting image, but I'm only seeing the first 225px or so on each row. The rest seems to be transparent. See https://imgur.com/an4WLhW.png as an example of a screenshot taken using the CDP Page.screencast API, passing a 903px width in the CDP request.
Patch Set #1, Line 928:
if (!video_frame.get() || video_frame->natural_size().IsEmpty()) {Probably we only want to relax this to include the ARGB format.
Done
To view, visit change 2593354. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
File media/renderers/paint_canvas_video_renderer.cc:
Patch Set #1, Line 392:
for (size_t i = chunk_start; i < chunk_start + rows; ++i) {
I don't think this is related to this because I'm seeing all of the rows in the resulting image, but […]
Ah that's because width should be multiplied by 4 since these are ARGB pixels.
To view, visit change 2593354. To unsubscribe, or for help writing mail filters, visit settings.
Andrey Kosyakov (Gerrit)
Attention is currently required from: Dale Curtis, Patrick Brosset.
1 comment:
File media/renderers/paint_canvas_video_renderer.cc:
Patch Set #1, Line 392:
for (size_t i = chunk_start; i < chunk_start + rows; ++i) {
Ah that's because width should be multiplied by 4 since these are ARGB pixels.
My suggestion is based on that we don't use i in the loop body (and chunk_start is factored into pixels), so I assume we only care about the number of iterations at this point, in which case subtracting any number from both sides of the loop condition expression should yield the same number of iterations, so we could as well do
for (size_t i = 0; i < rows; i++)
for brevity (or even "while (rows--)" :-))
To view, visit change 2593354. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Andrey Kosyakov, Patrick Brosset.
1 comment:
File media/renderers/paint_canvas_video_renderer.cc:
Patch Set #1, Line 392:
for (size_t i = chunk_start; i < chunk_start + rows; ++i) {
My suggestion is based on that we don't use i in the loop body (and chunk_start is factored into pix […]
Ah right, that's fine with me 😊
To view, visit change 2593354. To unsubscribe, or for help writing mail filters, visit settings.
Patrick Brosset (Gerrit)
Attention is currently required from: Dale Curtis, Andrey Kosyakov.
1 comment:
File media/renderers/paint_canvas_video_renderer.cc:
Patch Set #1, Line 392:
for (size_t i = chunk_start; i < chunk_start + rows; ++i) {
Ah right, that's fine with me 😊
Thank you both so much for the help. Will post an updated CL later today.
To view, visit change 2593354. To unsubscribe, or for help writing mail filters, visit settings.
Patrick Brosset (Gerrit)
Attention is currently required from: Dale Curtis, Andrey Kosyakov.
Patch set 3:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Attention is currently required from: Dale Curtis, Andrey Kosyakov.
CQ is trying the patch.
Note: The patchset #3 "addressed comments" sent to CQ was uploaded after this CL was CR+1-ed.
Reviewer, please verify there is nothing unexpected https://chromium-review.googlesource.com/c/2593354/3
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/2593354/3
Bot data: {"action": "start", "triggered_at": "2020-12-17T10:56:20.0Z", "revision": "4b0022b5b2ab316f4f66b7e1e4aad1cd1f1ebbac"}
Chromium LUCI CQ (Gerrit)
Attention is currently required from: Dale Curtis, Andrey Kosyakov.
Failed builds:
luci.chromium.try/linux_chromium_asan_rel_ng JOB_FAILED https://cr-buildbucket.appspot.com/build/8860671067863779552
1 Test Suite(s) failed.
**content_unittests** failed because of:
- DevToolsVideoConsumerTest.StartCaptureCallsSetFunctions
- DevToolsVideoConsumerTest.CapturerIsPassedCachedValues
Chromium LUCI CQ (Gerrit)
Attention is currently required from: Dale Curtis, Andrey Kosyakov.
CQ is trying the patch.
Note: The patchset #4 "CDP: Make colors in Page.screencast API accurate" sent to CQ was uploaded after this CL was CR+1-ed.
Reviewer, please verify there is nothing unexpected https://chromium-review.googlesource.com/c/2593354/4
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/2593354/4
Bot data: {"action": "start", "triggered_at": "2020-12-17T15:11:27.0Z", "revision": "ef526cf9e1bcf7620c140d4abab4635e8b174c1f"}
Patrick Brosset (Gerrit)
Attention is currently required from: Dale Curtis, Andrey Kosyakov.
Patch set 4:Commit-Queue +2