media: Plumb 10-bit input and HDR metadata for HEVC Main10 [chromium/src : main]
Dale Curtis (Gerrit)
Dale Curtis added 4 comments![]( "Open in Gerrit")
if (src_frame.format() == PIXEL_FORMAT_P010LE) {Can drop this now since libyuv is just calling the same thing you are below.
return frame.format();Sida ZhuHow are we hitting this path? Readback for HBD isn't implemented AFAIK?
Dale CurtisDecode 10bit P010 video and encode it to 8bit NV12 need this.
Saifuddin HitawalaHmm, I'm surprised this works at all. I guess the method is reading per-plane and thus avoids needing to deal with format intricacies. @hita...@chromium.org in case there's subtleties to beware of here.
Sida ZhuYes, it currently does readback per planes. I think it should be fine for readback for P010. But now that it is software P010 frame it could reach VideoResourceUpdater, and there we don't [support P010](https://source.chromium.org/chromium/chromium/src/+/main:media/renderers/video_resource_updater.cc;drc=1ab8a80b7593885d06d31204c595d9f9358e56be;l=282). Looks like it is also broken (or maybe fine?) for NV16/NV24 there since they are also not supported.
Dale Curtis`ReadbackFormat` is only used by texture/opaque SharedImage → CPU readback (`ReadbackTextureBackedFrameToMemorySync`), not by GMB map (ConvertToMemoryMappedFrame).
The hit path for this CL:
```
HW decode (Main10)
|
v
VideoFrame format = P010
storage = opaque SharedImage (HasSharedImage, !HasMappableSharedImage)
|
v
VideoEncoder encode as HEVC Main / NV12
DoesSupportGpuSharedImages(P010) == false
(gpu_supported_pixel_formats_ is NV12-only for Main)
|
v
StartReadback()
|
v
ReadbackTextureBackedFrameToMemorySync()
|
v
ReadbackFormat(frame) <── needs case P010LE
| else PIXEL_FORMAT_UNKNOWN → encode fails
v
per-plane ReadbackImagePixels (Y: R16, UV: R16G16)
|
v
CPU P010 frame
|
v
ConvertAndScale(P010 → NV12) → VEA```
Without the P010LE case, that opaque-SI P010 frame can’t leave GPU memory, so 10-bit decode → 8-bit encode breaks whenever the decoder doesn’t hand us a mappable GMB.It doesn’t go through RGB/VideoResourceUpdater. `SkColorTypeForPlane` already maps `P010/P210/P410` → `kR16_unorm` / `kR16G16_unorm`.
`P210LE` / `P410LE` should be similar like `P010LE`, so I've updated `ConvertAndScalePx10` and `ReadbackFormat` lists them for the same opaque-SI → CPU bridge (422/444), the HW decoder currently support decoder HEVC 4:2:2 & 4:4:4 10bit video into these format.
Acknowledged
// ConvertAndScale staging. VEA_READ_CAMERA_AND_CPU_READ_WRITE is onlyInteresting. Was this path totally failing before this fix?
// If the frame cannot be passed through, PrepareGpuFrame must allocate aSida ZhuDo we need this once the supported formats list is fixed? I don't follow why the existing code wouldn't already handle this path otherwise.
It also seems like l.619 would ensure `use_gpu_buffer` is false already given l.627.
Dale CurtisThe use_gpu_buffer = false block was a workaround for broken GPU staging on non-Ozone, not a format-list issue.
After the [opaque-SI CL]((https://chromium-review.googlesource.com/c/chromium/src/+/8174784), `supports_gpu_shared_images` makes many SI frames take the GPU path. If the format can’t passthrough, `PrepareGpuFrame` must allocate a `ConvertAndScale` destination with: `BufferUsage::VEA_READ_CAMERA_AND_CPU_READ_WRITE`.
On Mac that usage is rejected by both factories:
```
PrepareGpuFrame needs staging GMB
|
v
CreateSharedImage(..., VEA_READ_CAMERA_AND_CPU_READ_WRITE)
|
+---------------+----------------+
| |
v v
IsNativeBufferSupported() SharedMemory fallback
(Apple / IOSurface) IsBufferUsageSupported()
| |
v v
return false return false
| |
+---------------+----------------+
|
v
CreateSharedImage fails
|
v
Encode fails
```So: SI frame → can’t passthrough → `PrepareGpuFrame` → staging create fails.
The block forced those cases onto `PrepareCpuFrame` before staging was attempted:
```
use_gpu_buffer &&
preference != GpuMemBuf && // Linux GpuMemBuf still OK (Ozone/GBM)
!CanPassthrough(...)
→ use_gpu_buffer = false // avoid the dead Mac staging path
```L619 (CpuMemBuf preference) is different — it only covers the explicitly-CPU preference. This block covered the “SI frame, default/heuristic GPU path, needs conversion” case.
With staging switched to `GPU_READ_CPU_READ_WRITE`, that workaround is obsolete and can be removed.
Acknowledged
Related details
- Saifuddin Hitawala
- Sida Zhu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sida Zhu (Gerrit)
Sida Zhu added 2 comments![]( "Open in Gerrit")
Can drop this now since libyuv is just calling the same thing you are below.
Yeah, you are right, done.
// ConvertAndScale staging. VEA_READ_CAMERA_AND_CPU_READ_WRITE is onlyInteresting. Was this path totally failing before this fix?
Not every encode hit this path, but yes: when the adapter needed a mappable
SharedImage staging buffer for `ConvertAndScale()`, `VEA_READ_CAMERA_AND_CPU_READ_WRITE`
was only supported by the Ozone/GBM path. Windows/Android native backings reject
that usage, and the shared-memory backing rejects it as well. The path is hit
when GPU input is selected but the source frame cannot be passed through
directly, e.g. because it needs resize or format conversion.
Related details
- Dale Curtis
- Saifuddin Hitawala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
2 comments
We could do better for P0x -> P010 w/o scaling, but this is fine for now since I expect this to be rare.
// ConvertAndScale staging. VEA_READ_CAMERA_AND_CPU_READ_WRITE is onlySida ZhuInteresting. Was this path totally failing before this fix?
Not every encode hit this path, but yes: when the adapter needed a mappable
SharedImage staging buffer for `ConvertAndScale()`, `VEA_READ_CAMERA_AND_CPU_READ_WRITE`
was only supported by the Ozone/GBM path. Windows/Android native backings reject
that usage, and the shared-memory backing rejects it as well. The path is hit
when GPU input is selected but the source frame cannot be passed through
directly, e.g. because it needs resize or format conversion.
- Saifuddin Hitawala
- Sida Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qiu, Jianlin (Gerrit)
Qiu, Jianlin added 1 comment![]( "Open in Gerrit")
EncoderStatus VideoFrameConverter::ConvertAndScalePx10(
const VideoFrame* src_frame,
VideoFrame& dest_frame) {
DCHECK(src_frame->format() == PIXEL_FORMAT_P010LE ||
src_frame->format() == PIXEL_FORMAT_P210LE ||
src_frame->format() == PIXEL_FORMAT_P410LE);
dest_frame.set_color_space(src_frame->ColorSpace());
if (src_frame->format() == PIXEL_FORMAT_P010LE &&
(dest_frame.format() == PIXEL_FORMAT_NV12 ||
dest_frame.format() == PIXEL_FORMAT_NV12A) &&
src_frame->visible_rect().size() == dest_frame.visible_rect().size()) {
return internals::P010ToNV12x(*src_frame, dest_frame)
? OkStatus()
: EncoderStatus(EncoderStatus::Codes::kFormatConversionError);
}Any possiblility we utilize https://developer.apple.com/documentation/videotoolbox/vtpixeltransfersession-api-collection for resolving the shared image, or doing the conversion for non-shared-image in VTVEA with hardware acceleration?
It is OK you implement that as part of CL:8174784. Just would like to know if there is known platform limitation.
Related details
- Saifuddin Hitawala
- Sida Zhu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qiu, Jianlin (Gerrit)
Qiu, Jianlin voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Qiu, Jianlin (Gerrit)
Qiu, Jianlin voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Saifuddin Hitawala
- Sida Zhu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sida Zhu (Gerrit)
Sida Zhu added 1 comment![]( "Open in Gerrit")
EncoderStatus VideoFrameConverter::ConvertAndScalePx10(
const VideoFrame* src_frame,
VideoFrame& dest_frame) {
DCHECK(src_frame->format() == PIXEL_FORMAT_P010LE ||
src_frame->format() == PIXEL_FORMAT_P210LE ||
src_frame->format() == PIXEL_FORMAT_P410LE);
dest_frame.set_color_space(src_frame->ColorSpace());
if (src_frame->format() == PIXEL_FORMAT_P010LE &&
(dest_frame.format() == PIXEL_FORMAT_NV12 ||
dest_frame.format() == PIXEL_FORMAT_NV12A) &&
src_frame->visible_rect().size() == dest_frame.visible_rect().size()) {
return internals::P010ToNV12x(*src_frame, dest_frame)
? OkStatus()
: EncoderStatus(EncoderStatus::Codes::kFormatConversionError);
}Any possiblility we utilize https://developer.apple.com/documentation/videotoolbox/vtpixeltransfersession-api-collection for resolving the shared image, or doing the conversion for non-shared-image in VTVEA with hardware acceleration?
It is OK you implement that as part of CL:8174784. Just would like to know if there is known platform limitation.
That's another solution, different from the current VideoFrameConverter path (GPU scaling instead of CPU). It should technically deliver better performance for pixel‑format conversion. For cases without pixel‑format conversion, scaling and crop are already supported in the opaque SI zero copy CL — this is already an improvement over the old path that fully relied on VideoFrameConverter. We can explore its feasibility in a follow‑up threads.
Related details
- Qiu, Jianlin
- Saifuddin Hitawala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qiu, Jianlin (Gerrit)
Qiu, Jianlin added 1 comment![]( "Open in Gerrit")
EncoderStatus VideoFrameConverter::ConvertAndScalePx10(
const VideoFrame* src_frame,
VideoFrame& dest_frame) {
DCHECK(src_frame->format() == PIXEL_FORMAT_P010LE ||
src_frame->format() == PIXEL_FORMAT_P210LE ||
src_frame->format() == PIXEL_FORMAT_P410LE);
dest_frame.set_color_space(src_frame->ColorSpace());
if (src_frame->format() == PIXEL_FORMAT_P010LE &&
(dest_frame.format() == PIXEL_FORMAT_NV12 ||
dest_frame.format() == PIXEL_FORMAT_NV12A) &&
src_frame->visible_rect().size() == dest_frame.visible_rect().size()) {
return internals::P010ToNV12x(*src_frame, dest_frame)
? OkStatus()
: EncoderStatus(EncoderStatus::Codes::kFormatConversionError);
}Sida ZhuAny possiblility we utilize https://developer.apple.com/documentation/videotoolbox/vtpixeltransfersession-api-collection for resolving the shared image, or doing the conversion for non-shared-image in VTVEA with hardware acceleration?
It is OK you implement that as part of CL:8174784. Just would like to know if there is known platform limitation.
That's another solution, different from the current VideoFrameConverter path (GPU scaling instead of CPU). It should technically deliver better performance for pixel‑format conversion. For cases without pixel‑format conversion, scaling and crop are already supported in the opaque SI zero copy CL — this is already an improvement over the old path that fully relied on VideoFrameConverter. We can explore its feasibility in a follow‑up threads.
Thanks! I would like ARGB inputs to be handled without a readback. Looking forward to see the feasibility.
Related details
- Saifuddin Hitawala
- Sida Zhu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |