[media] Update NativePixmapFrameResource to handle ColorSpace changes. [chromium/src : main]
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 3 comments![]( "Open in Gerrit")
Updated to make some changes addressing feedback. Please take another look.
base::FeatureList::IsEnabled(kV4L2VEAUseCorrectColorSpace)
? input_color_space_
: gfx::ColorSpace(),Saifuddin HitawalaDo you see anything changed by this CL?
The existing code propagates the input color space.
https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/chromeos/libyuv_image_processor_backend.cc;l=352;drc=9405179380f3b5222471aa3430a8683f8c88f137Besides, V4L2VEA doesn't currently embed the color space information to the stream.
Saifuddin HitawalaSorry for the delay as I'm currently OOO. Yes, it propagates the invalid color space there, and since earlier we set a default color space on output frame's SharedImage in platform_video_frame_utils.cc, we see a mismatch triggered. With this CL we don't see a mismatch triggered as both the SharedImage created for output frame as well as on the input frame we have some defaults.
Besides, V4L2VEA doesn't currently embed the color space information to the stream.
That is something I hadn't considered. In that case it doesn't make sense to store `input_color_space_`. I can look into it once I'm back.
Updated to remove these changes from V4L2VEA.
input_color_space_ = frame.ColorSpace();Hirokazu HondaIdeally, this should default here to Rec709 or SRGB if `frame.ColorSpace()` is invalid and also set this default ColorSpace on `frame`, to ensure SharedImage always has a valid ColorSpace on creation. I'm not doing that here now, and we can look into it later as part of later goal.
Saifuddin HitawalaDo you have any plan to do this?
The current code is nothing is set in V4L2VEA::Initialize(), so input_color_space_ is invalid.
No IP reconfiguration happens if no frame color space is specified.
Saifuddin HitawalaYes, I could look into doing this as well setting the default color space on `frame`.
I've reverted these changes here on further thought and now am just passing a default color space so the behaviour is same as before we added defaults (so that the output frame's color space is not overridden to BT709 in platform utils) and now default setting is moved up the stack. This still helps us meet our current goal of ensuring that the color spaces always match between shared image and video frame.
I can look into setting a default color space for input/output frames for V4L2VEA at a later point while evaluating other creators of SharedImage for its feasibility.
Related details
- Hirokazu Honda
- Pilar Molina Lopez
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 1 comment![]( "Open in Gerrit")
Gentle ping.
I've reverted changes to V4L2VEA and now just passing a default color space so the behaviour is same as before we added defaults. This still helps us meet our current goal of ensuring that the color spaces always match between shared image and video frame. This patch now moves the default up from platform utils to NativePixmapFrameResource which sets a default color space.
Please take another look.
Hirokazu Honda (Gerrit)
Hirokazu Honda voted Code-Review+1![]( "Open in Gerrit")
- Pilar Molina Lopez
- Saifuddin Hitawala
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hirokazu Honda (Gerrit)
Hirokazu Honda added 1 comment![]( "Open in Gerrit")
Sorry for the late response. LGTM.
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala voted and added 1 comment![]( "Open in Gerrit")
Votes added by Saifuddin Hitawala
| Commit-Queue | +2 |
1 comment
- Pilar Molina Lopez
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[media] Update NativePixmapFrameResource to handle ColorSpace changes.
This change updates platform VF utility methods -
CreateVideoFrameFromGpuMemoryBufferHandle and
CreateMappableSharedImageVideoFrame to take in ColorSpace and not
pass any default ColorSpaces, and let the client calling these
utilities pass the proper ColorSpace and default if needed. As part of
this, NativePixmapFrameResource now passes a default ColorSpace
in similar manner behind a killswitch. The other callsite is
V4L2VideoEncodeAccelerator which passes the input frame's default
color space.
Note these defaults in platform VideoFrame utils were added by us as
part of larger refactor to ensure that VideoFrames backed by
SharedImages have the same ColorSpace for both SharedImage and
VideoFrame, and creators of VideoFrame pass a valid color space.
Also update various tests to pass in a proper color space.
- M media/gpu/chromeos/image_processor_perf_test.cc
- M media/gpu/chromeos/image_processor_test.cc
- M media/gpu/chromeos/native_pixmap_frame_resource.cc
- M media/gpu/chromeos/platform_video_frame_utils.cc
- M media/gpu/chromeos/platform_video_frame_utils.h
- M media/gpu/chromeos/platform_video_frame_utils_unittest.cc
- M media/gpu/chromeos/vulkan_overlay_adaptor_test.cc
- M media/gpu/test/video_frame_helpers.cc
- M media/gpu/test/video_player/test_vda_video_decoder.cc
- M media/gpu/v4l2/v4l2_video_encode_accelerator.cc
Code-Review: +1 by Hirokazu Honda
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |