From e6e93d8cedefcae0a6efd07067279eef76d3ff71 Mon Sep 17 00:00:00 2001 From: Dale Curtis Date: Wed, 15 Mar 2023 21:20:27 +0000 Subject: [PATCH] Reland: "Update macOS VideoToolbox encoder color space settings." This ensures the right color space information is set in the bitstream parameters. Sadly changing them while encodes are pending can hang the encoding session, so color space changes force creation of a new VTCompressionSession. This also cleans up the tests pretty substantially by adding an assert_eq() method for easier bot debugging. It also swaps the RGBA then I420 to I420 first since macOS defaults to a SRGB/rec601 color space when none is provided (so tests would pass w/o this change). No other hardware encoder sets a color space at all, but macOS has been silently assuming ~rec709 this whole time and seemingly converting frames to that space (see drawImage failures in earlier patch sets before WrapVideoFrameInCVPixelBuffer() fix). Since we set the same color space on inputs as we do the session, there should be no color space conversion happening by the OS. Bug: 1377842 Cq-Include-Trybots: luci.chromium.try:gpu-fyi-try-mac-nvidia-retina-rel,gpu-fyi-try-mac-intel-asan,gpu-fyi-try-mac-amd-retina-asan Change-Id: Ifa9fd481fc7197610418097d96cb5d733055088d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4246660 Reviewed-by: Alex Gough Commit-Queue: Dale Curtis Reviewed-by: Eugene Zemtsov Reviewed-by: Brian Sheedy Cr-Commit-Position: refs/heads/main@{#1117755} --- content/test/data/gpu/webcodecs/copyTo.html | 17 +- .../gpu/webcodecs/encode-color-space.html | 194 +++++++++++++----- .../data/gpu/webcodecs/webcodecs_common.js | 8 +- .../webcodecs_expectations.txt | 6 + media/base/mac/color_space_util_mac.h | 6 + media/base/mac/color_space_util_mac.mm | 111 +++++++--- media/base/mac/video_frame_mac.cc | 66 ++++-- .../mac/vt_video_encode_accelerator_mac.cc | 99 ++++++++- .../gpu/mac/vt_video_encode_accelerator_mac.h | 7 + .../mojo/mojom/video_encode_accelerator.mojom | 2 + .../video_encode_accelerator_mojom_traits.cc | 3 + .../video_encode_accelerator_mojom_traits.h | 4 + media/video/video_encode_accelerator.h | 4 + .../video/video_encode_accelerator_adapter.cc | 3 +- .../modules/webcodecs/video_encoder.cc | 14 +- 15 files changed, 431 insertions(+), 113 deletions(-) diff --git a/content/test/data/gpu/webcodecs/copyTo.html b/content/test/data/gpu/webcodecs/copyTo.html index ca1c829d14f0d..ede2c36b97976 100644 --- a/content/test/data/gpu/webcodecs/copyTo.html +++ b/content/test/data/gpu/webcodecs/copyTo.html @@ -7,13 +7,20 @@ diff --git a/content/test/data/gpu/webcodecs/webcodecs_common.js b/content/test/data/gpu/webcodecs/webcodecs_common.js index bffd6aa7a0734..fdbc654d4e9ea 100644 --- a/content/test/data/gpu/webcodecs/webcodecs_common.js +++ b/content/test/data/gpu/webcodecs/webcodecs_common.js @@ -37,6 +37,12 @@ class TestHarness { this.reportFailure("Assertion failed: " + msg); } + assert_eq(val1, val2, msg) { + if (val1 != val2) { + this.reportFailure(`Assertion failed: ${msg}. ${val1} != ${val2}.`); + } + } + summary() { return this.message + "\n\n" + this.logs.join("\n"); } @@ -438,4 +444,4 @@ async function createFrameSource(type, width, height) { return new ArrayBufferSource(width, height); } } -} \ No newline at end of file +} diff --git a/content/test/gpu/gpu_tests/test_expectations/webcodecs_expectations.txt b/content/test/gpu/gpu_tests/test_expectations/webcodecs_expectations.txt index 0de74e0b7aa6d..b262110c40dcb 100644 --- a/content/test/gpu/gpu_tests/test_expectations/webcodecs_expectations.txt +++ b/content/test/gpu/gpu_tests/test_expectations/webcodecs_expectations.txt @@ -115,6 +115,12 @@ crbug.com/1400512 [ fuchsia fuchsia-board-sherlock ] WebCodecs_copyTo_camera [ F # finder:enable-unused +# Temporary expectation while hardware encoder color spaces are added. +crbug.com/1377842 [ win ] WebCodecs_EncodeColorSpace_avc1.42001E_prefer-hardware [ Failure ] +crbug.com/1377842 [ android ] WebCodecs_EncodeColorSpace_avc1.42001E_prefer-hardware [ Failure ] +crbug.com/1377842 [ android ] WebCodecs_EncodeColorSpace_avc1.42001E_prefer-software [ Failure ] +crbug.com/1377842 [ android ] WebCodecs_EncodeColorSpace_vp09.00.10.08_prefer-hardware [ Failure ] + ####################################################################### # Automated Entries After This Point - Do Not Manually Add Below Here # ####################################################################### diff --git a/media/base/mac/color_space_util_mac.h b/media/base/mac/color_space_util_mac.h index 321747964daff..c2f58e87a235f 100644 --- a/media/base/mac/color_space_util_mac.h +++ b/media/base/mac/color_space_util_mac.h @@ -21,6 +21,12 @@ MEDIA_EXPORT gfx::ColorSpace GetImageBufferColorSpace( MEDIA_EXPORT gfx::ColorSpace GetFormatDescriptionColorSpace( CMFormatDescriptionRef format_description); +// Converts a gfx::ColorSpace to individual kCVImageBuffer* keys. +MEDIA_EXPORT bool GetImageBufferColorValues(const gfx::ColorSpace& color_space, + CFStringRef* out_primaries, + CFStringRef* out_transfer, + CFStringRef* out_matrix); + } // namespace media #endif // MEDIA_BASE_MAC_COLOR_SPACE_UTIL_MAC_H_ diff --git a/media/base/mac/color_space_util_mac.mm b/media/base/mac/color_space_util_mac.mm index d151cd8ccc9e4..ecd0f91f02b28 100644 --- a/media/base/mac/color_space_util_mac.mm +++ b/media/base/mac/color_space_util_mac.mm @@ -42,12 +42,12 @@ bool GetImageBufferProperty(CFTypeRef value_untyped, return false; } -gfx::ColorSpace::PrimaryID GetCoreVideoPrimary(CFTypeRef primaries_untyped) { - struct CVImagePrimary { - const CFStringRef cfstr_cv; - const CFStringRef cfstr_cm; - const gfx::ColorSpace::PrimaryID id; - }; +struct CVImagePrimary { + const CFStringRef cfstr_cv; + const CFStringRef cfstr_cm; + const gfx::ColorSpace::PrimaryID id; +}; +const std::vector& GetSupportedImagePrimaries() { static const base::NoDestructor> kSupportedPrimaries([] { std::vector supported_primaries; @@ -69,24 +69,26 @@ bool GetImageBufferProperty(CFTypeRef value_untyped, gfx::ColorSpace::PrimaryID::BT2020}); return supported_primaries; }()); + return *kSupportedPrimaries; +} +gfx::ColorSpace::PrimaryID GetCoreVideoPrimary(CFTypeRef primaries_untyped) { // The named primaries. Default to BT709. auto primary_id = gfx::ColorSpace::PrimaryID::BT709; - if (!GetImageBufferProperty(primaries_untyped, *kSupportedPrimaries, + if (!GetImageBufferProperty(primaries_untyped, GetSupportedImagePrimaries(), &primary_id)) { - DLOG(ERROR) << "Failed to find CVImageBufferRef primaries."; + DLOG(ERROR) << "Failed to find CVImageBufferRef primaries: " + << primaries_untyped; } return primary_id; } -gfx::ColorSpace::TransferID GetCoreVideoTransferFn(CFTypeRef transfer_untyped, - CFTypeRef gamma_untyped, - double* gamma) { - struct CVImageTransferFn { - const CFStringRef cfstr_cv; - const CFStringRef cfstr_cm; - const gfx::ColorSpace::TransferID id; - }; +struct CVImageTransferFn { + const CFStringRef cfstr_cv; + const CFStringRef cfstr_cm; + const gfx::ColorSpace::TransferID id; +}; +const std::vector& GetSupportedImageTransferFn() { static const base::NoDestructor> kSupportedTransferFuncs([] { std::vector supported_transfer_funcs; @@ -94,6 +96,14 @@ bool GetImageBufferProperty(CFTypeRef value_untyped, {kCVImageBufferTransferFunction_ITU_R_709_2, kCMFormatDescriptionTransferFunction_ITU_R_709_2, gfx::ColorSpace::TransferID::BT709_APPLE}); + supported_transfer_funcs.push_back( + {kCVImageBufferTransferFunction_ITU_R_709_2, + kCMFormatDescriptionTransferFunction_ITU_R_709_2, + gfx::ColorSpace::TransferID::BT709}); + supported_transfer_funcs.push_back( + {kCVImageBufferTransferFunction_ITU_R_709_2, + kCMFormatDescriptionTransferFunction_ITU_R_709_2, + gfx::ColorSpace::TransferID::SMPTE170M}); supported_transfer_funcs.push_back( {kCVImageBufferTransferFunction_SMPTE_240M_1995, kCMFormatDescriptionTransferFunction_SMPTE_240M_1995, @@ -136,12 +146,18 @@ bool GetImageBufferProperty(CFTypeRef value_untyped, return supported_transfer_funcs; }()); + return *kSupportedTransferFuncs; +} +gfx::ColorSpace::TransferID GetCoreVideoTransferFn(CFTypeRef transfer_untyped, + CFTypeRef gamma_untyped, + double* gamma) { // The named transfer function. auto transfer_id = gfx::ColorSpace::TransferID::BT709; - if (!GetImageBufferProperty(transfer_untyped, *kSupportedTransferFuncs, + if (!GetImageBufferProperty(transfer_untyped, GetSupportedImageTransferFn(), &transfer_id)) { - DLOG(ERROR) << "Failed to find CVImageBufferRef transfer."; + DLOG(ERROR) << "Failed to find CVImageBufferRef transfer: " + << transfer_untyped; } if (transfer_id != gfx::ColorSpace::TransferID::CUSTOM) @@ -171,12 +187,12 @@ bool GetImageBufferProperty(CFTypeRef value_untyped, return transfer_id; } -gfx::ColorSpace::MatrixID GetCoreVideoMatrix(CFTypeRef matrix_untyped) { - struct CVImageMatrix { - const CFStringRef cfstr_cv; - const CFStringRef cfstr_cm; - gfx::ColorSpace::MatrixID id; - }; +struct CVImageMatrix { + const CFStringRef cfstr_cv; + const CFStringRef cfstr_cm; + gfx::ColorSpace::MatrixID id; +}; +const std::vector& GetSupportedImageMatrix() { static const base::NoDestructor> kSupportedMatrices([] { std::vector supported_matrices; @@ -198,11 +214,15 @@ bool GetImageBufferProperty(CFTypeRef value_untyped, gfx::ColorSpace::MatrixID::BT2020_NCL}); return supported_matrices; }()); + return *kSupportedMatrices; +} +gfx::ColorSpace::MatrixID GetCoreVideoMatrix(CFTypeRef matrix_untyped) { auto matrix_id = gfx::ColorSpace::MatrixID::INVALID; - if (!GetImageBufferProperty(matrix_untyped, *kSupportedMatrices, + if (!GetImageBufferProperty(matrix_untyped, GetSupportedImageMatrix(), &matrix_id)) { - DLOG(ERROR) << "Failed to find CVImageBufferRef YUV matrix."; + DLOG(ERROR) << "Failed to find CVImageBufferRef YUV matrix: " + << matrix_untyped; } return matrix_id; } @@ -299,4 +319,43 @@ bool GetImageBufferProperty(CFTypeRef value_untyped, format_description, kCMFormatDescriptionExtension_YCbCrMatrix)); } +// Converts a gfx::ColorSpace to individual kCVImageBuffer* keys. +bool GetImageBufferColorValues(const gfx::ColorSpace& color_space, + CFStringRef* out_primaries, + CFStringRef* out_transfer, + CFStringRef* out_matrix) { + DCHECK(out_primaries); + DCHECK(out_transfer); + DCHECK(out_matrix); + + bool found_primary = false; + for (const auto& primaries : GetSupportedImagePrimaries()) { + if (primaries.id == color_space.GetPrimaryID()) { + *out_primaries = primaries.cfstr_cv; + found_primary = true; + break; + } + } + + bool found_transfer = false; + for (const auto& transfer : GetSupportedImageTransferFn()) { + if (transfer.id == color_space.GetTransferID()) { + *out_transfer = transfer.cfstr_cv; + found_transfer = true; + break; + } + } + + bool found_matrix = false; + for (const auto& matrix : GetSupportedImageMatrix()) { + if (matrix.id == color_space.GetMatrixID()) { + *out_matrix = matrix.cfstr_cv; + found_matrix = true; + break; + } + } + + return found_primary && found_transfer && found_matrix; +} + } // namespace media diff --git a/media/base/mac/video_frame_mac.cc b/media/base/mac/video_frame_mac.cc index 5c543a95b3028..2ff54da872f62 100644 --- a/media/base/mac/video_frame_mac.cc +++ b/media/base/mac/video_frame_mac.cc @@ -10,6 +10,8 @@ #include #include "base/logging.h" +#include "base/strings/sys_string_conversions.h" +#include "media/base/mac/color_space_util_mac.h" #include "media/base/video_frame.h" #include "media/base/video_util.h" #include "ui/gfx/gpu_memory_buffer.h" @@ -49,6 +51,40 @@ bool IsAcceptableCvPixelFormat(VideoPixelFormat format, OSType cv_format) { return false; } +bool CvPixelBufferHasColorSpace(CVPixelBufferRef pixel_buffer) { + return CVBufferGetAttachment(pixel_buffer, kCVImageBufferColorPrimariesKey, + nullptr) && + CVBufferGetAttachment(pixel_buffer, kCVImageBufferTransferFunctionKey, + nullptr) && + CVBufferGetAttachment(pixel_buffer, kCVImageBufferYCbCrMatrixKey, + nullptr); +} + +void SetCvPixelBufferColorSpace(const gfx::ColorSpace& frame_cs, + CVPixelBufferRef pixel_buffer) { + // Apply required colorimetric attachments. + CFStringRef primary, transfer, matrix; + if (frame_cs.IsValid() && + GetImageBufferColorValues(frame_cs, &primary, &transfer, &matrix)) { + CVBufferSetAttachment(pixel_buffer, kCVImageBufferColorPrimariesKey, + primary, kCVAttachmentMode_ShouldPropagate); + CVBufferSetAttachment(pixel_buffer, kCVImageBufferTransferFunctionKey, + transfer, kCVAttachmentMode_ShouldPropagate); + CVBufferSetAttachment(pixel_buffer, kCVImageBufferYCbCrMatrixKey, matrix, + kCVAttachmentMode_ShouldPropagate); + } else if (!CvPixelBufferHasColorSpace(pixel_buffer)) { + CVBufferSetAttachment(pixel_buffer, kCVImageBufferColorPrimariesKey, + kCVImageBufferColorPrimaries_ITU_R_709_2, + kCVAttachmentMode_ShouldPropagate); + CVBufferSetAttachment(pixel_buffer, kCVImageBufferTransferFunctionKey, + kCVImageBufferTransferFunction_ITU_R_709_2, + kCVAttachmentMode_ShouldPropagate); + CVBufferSetAttachment(pixel_buffer, kCVImageBufferYCbCrMatrixKey, + kCVImageBufferYCbCrMatrix_ITU_R_709_2, + kCVAttachmentMode_ShouldPropagate); + } +} + } // namespace MEDIA_EXPORT base::ScopedCFTypeRef @@ -68,6 +104,8 @@ WrapVideoFrameInCVPixelBuffer(scoped_refptr frame) { frame->format(), CVPixelBufferGetPixelFormatType(pixel_buffer))) { DLOG(ERROR) << "Dropping CVPixelBuffer w/ incorrect format."; pixel_buffer.reset(); + } else { + SetCvPixelBufferColorSpace(frame->ColorSpace(), pixel_buffer); } return pixel_buffer; } @@ -90,6 +128,8 @@ WrapVideoFrameInCVPixelBuffer(scoped_refptr frame) { CVPixelBufferGetPixelFormatType(pixel_buffer))) { DLOG(ERROR) << "Dropping CVPixelBuffer w/ incorrect format."; pixel_buffer.reset(); + } else { + SetCvPixelBufferColorSpace(frame->ColorSpace(), pixel_buffer); } return pixel_buffer; } @@ -104,20 +144,25 @@ WrapVideoFrameInCVPixelBuffer(scoped_refptr frame) { if (!frame) return pixel_buffer; - VLOG(3) << "Returning RAM based CVPixelBuffer."; - // VideoFrame only supports YUV formats and most of them are 'YVU' ordered, // which CVPixelBuffer does not support. This means we effectively can only // represent I420 and NV12 frames. In addition, VideoFrame does not carry // colorimetric information, so this function assumes standard video range // and ITU Rec 709 primaries. const VideoPixelFormat video_frame_format = frame->format(); + const bool is_full_range = + frame->ColorSpace().GetRangeID() == gfx::ColorSpace::RangeID::FULL; OSType cv_format; if (video_frame_format == PIXEL_FORMAT_I420) { - cv_format = kCVPixelFormatType_420YpCbCr8Planar; + cv_format = is_full_range ? kCVPixelFormatType_420YpCbCr8PlanarFullRange + : kCVPixelFormatType_420YpCbCr8Planar; } else if (video_frame_format == PIXEL_FORMAT_NV12) { - cv_format = kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange; + cv_format = is_full_range ? kCVPixelFormatType_420YpCbCr8BiPlanarFullRange + : kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange; } else if (video_frame_format == PIXEL_FORMAT_NV12A) { + if (is_full_range) { + DVLOG(1) << "Full range NV12A is not supported by the OS."; + } cv_format = kCVPixelFormatType_420YpCbCr8VideoRange_8A_TriPlanar; } else { DLOG(ERROR) << "Unsupported frame format: " << video_frame_format; @@ -166,18 +211,7 @@ WrapVideoFrameInCVPixelBuffer(scoped_refptr frame) { // reference count manually. The release callback set on the pixel buffer will // release the frame. frame->AddRef(); - - // Apply required colorimetric attachments. - CVBufferSetAttachment(pixel_buffer, kCVImageBufferColorPrimariesKey, - kCVImageBufferColorPrimaries_ITU_R_709_2, - kCVAttachmentMode_ShouldPropagate); - CVBufferSetAttachment(pixel_buffer, kCVImageBufferTransferFunctionKey, - kCVImageBufferTransferFunction_ITU_R_709_2, - kCVAttachmentMode_ShouldPropagate); - CVBufferSetAttachment(pixel_buffer, kCVImageBufferYCbCrMatrixKey, - kCVImageBufferYCbCrMatrix_ITU_R_709_2, - kCVAttachmentMode_ShouldPropagate); - + SetCvPixelBufferColorSpace(frame->ColorSpace(), pixel_buffer); return pixel_buffer; } diff --git a/media/gpu/mac/vt_video_encode_accelerator_mac.cc b/media/gpu/mac/vt_video_encode_accelerator_mac.cc index b8d54484fd856..5d098d9bc6b39 100644 --- a/media/gpu/mac/vt_video_encode_accelerator_mac.cc +++ b/media/gpu/mac/vt_video_encode_accelerator_mac.cc @@ -19,6 +19,7 @@ #include "build/build_config.h" #include "media/base/bitrate.h" #include "media/base/bitstream_buffer.h" +#include "media/base/mac/color_space_util_mac.h" #include "media/base/mac/video_frame_mac.h" #include "media/base/media_log.h" #include "media/base/media_switches.h" @@ -195,10 +196,12 @@ VideoEncoderInfo GetVideoEncoderInfo(VTSessionRef compression_session, } // namespace struct VTVideoEncodeAccelerator::InProgressFrameEncode { - InProgressFrameEncode(base::TimeDelta rtp_timestamp) - : timestamp(rtp_timestamp) {} + InProgressFrameEncode(base::TimeDelta rtp_timestamp, + const gfx::ColorSpace& frame_cs) + : timestamp(rtp_timestamp), encoded_color_space(frame_cs) {} const base::TimeDelta timestamp; + const gfx::ColorSpace encoded_color_space; }; struct VTVideoEncodeAccelerator::EncodeOutput { @@ -206,10 +209,11 @@ struct VTVideoEncodeAccelerator::EncodeOutput { EncodeOutput(VTEncodeInfoFlags info_flags, CMSampleBufferRef sbuf, - base::TimeDelta timestamp) + const InProgressFrameEncode& frame_info) : info(info_flags), sample_buffer(sbuf, base::scoped_policy::RETAIN), - capture_timestamp(timestamp) {} + capture_timestamp(frame_info.timestamp), + encoded_color_space(frame_info.encoded_color_space) {} EncodeOutput(const EncodeOutput&) = delete; EncodeOutput& operator=(const EncodeOutput&) = delete; @@ -217,6 +221,7 @@ struct VTVideoEncodeAccelerator::EncodeOutput { const VTEncodeInfoFlags info; const base::ScopedCFTypeRef sample_buffer; const base::TimeDelta capture_timestamp; + const gfx::ColorSpace encoded_color_space; }; struct VTVideoEncodeAccelerator::BitstreamBufferRef { @@ -435,6 +440,39 @@ void VTVideoEncodeAccelerator::Encode(scoped_refptr frame, client_->NotifyError(kPlatformFailureError); return; } + + if (can_set_encoder_color_space_) { + // WrapVideoFrameInCVPixelBuffer() will do a few different things depending + // on the input buffer type: + // * If it's an IOSurface, the underlying attached color space will + // passthough to the pixel buffer. + // * If we're uploading to a new pixel buffer and the provided frame color + // space is valid that'll be set on the pixel buffer. + // * If the frame color space is not valid, BT709 will be assumed. + auto frame_cs = GetImageBufferColorSpace(pixel_buffer); + if (encoder_color_space_ && frame_cs != encoder_color_space_) { + if (pending_encodes_) { + auto status = VTCompressionSessionCompleteFrames(compression_session_, + kCMTimeInvalid); + if (status != noErr) { + OSSTATUS_LOG(ERROR, status) << " flush failed: " << status; + client_->NotifyError(kPlatformFailureError); + return; + } + } + if (!ResetCompressionSession(codec_)) { + client_->NotifyError(kPlatformFailureError); + return; + } + encoder_color_space_.reset(); + } + + if (!encoder_color_space_) { + encoder_color_space_ = frame_cs; + SetEncoderColorSpace(); + } + } + base::ScopedCFTypeRef frame_props = video_toolbox::DictionaryWithKeyValue( kVTEncodeFrameOptionKey_ForceKeyFrame, @@ -442,7 +480,8 @@ void VTVideoEncodeAccelerator::Encode(scoped_refptr frame, // Wrap information we'll need after the frame is encoded in a heap object. // We'll get the pointer back from the VideoToolbox completion callback. - auto request = std::make_unique(frame->timestamp()); + auto request = std::make_unique( + frame->timestamp(), encoder_color_space_.value_or(gfx::ColorSpace())); if (bitrate_.mode() == Bitrate::Mode::kConstant) { // In CBR mode, we adjust bitrate before every encode based on past history @@ -560,7 +599,7 @@ void VTVideoEncodeAccelerator::Flush(FlushCallback flush_callback) { if (status != noErr) { OSSTATUS_DLOG(ERROR, status) - << " VTCompressionSessionCompleteFrames failed: " << status; + << " VTCompressionSessionCompleteFrames failed: "; std::move(flush_callback).Run(/*success=*/false); return; } @@ -626,8 +665,7 @@ void VTVideoEncodeAccelerator::CompressionCallback(void* encoder_opaque, // EncodeOutput holds onto CMSampleBufferRef when posting task between // threads. - auto encode_output = - std::make_unique(info, sbuf, frame_info->timestamp); + auto encode_output = std::make_unique(info, sbuf, *frame_info); // This method is NOT called on |task_runner_|, so we still need to // post a task back to it to do work. @@ -647,7 +685,7 @@ void VTVideoEncodeAccelerator::CompressionCallbackTask( DCHECK_GE(pending_encodes_, 0); if (status != noErr) { - DLOG(ERROR) << " encode failed: " << status; + OSSTATUS_DLOG(ERROR, status) << "Encode failed: "; client_->NotifyError(kPlatformFailureError); return; } @@ -721,6 +759,8 @@ void VTVideoEncodeAccelerator::ReturnBitstreamBuffer( break; } + md.encoded_color_space = encode_output->encoded_color_space; + client_->BitstreamBufferReady(buffer_ref->id, std::move(md)); MaybeRunFlushCallback(); } @@ -871,4 +911,45 @@ void VTVideoEncodeAccelerator::MaybeRunFlushCallback() { std::move(pending_flush_cb_).Run(/*success=*/true); } +void VTVideoEncodeAccelerator::SetEncoderColorSpace() { + if (!encoder_color_space_ || !encoder_color_space_->IsValid()) { + return; + } + + CFStringRef primary, transfer, matrix; + if (!GetImageBufferColorValues(*encoder_color_space_, &primary, &transfer, + &matrix)) { + DLOG(ERROR) << "Failed to set bitstream color space: " + << encoder_color_space_->ToString(); + return; + } + + video_toolbox::SessionPropertySetter session_property_setter( + compression_session_); + if (!session_property_setter.IsSupported( + kVTCompressionPropertyKey_ColorPrimaries) || + !session_property_setter.IsSupported( + kVTCompressionPropertyKey_TransferFunction) || + !session_property_setter.IsSupported( + kVTCompressionPropertyKey_YCbCrMatrix)) { + DLOG(ERROR) << "VTCompressionSession doesn't support color space settings."; + can_set_encoder_color_space_ = false; + return; + } + + if (!session_property_setter.Set(kVTCompressionPropertyKey_ColorPrimaries, + primary) || + !session_property_setter.Set(kVTCompressionPropertyKey_TransferFunction, + transfer) || + !session_property_setter.Set(kVTCompressionPropertyKey_YCbCrMatrix, + matrix)) { + DLOG(ERROR) << "Failed to set color space on VTCompressionSession."; + can_set_encoder_color_space_ = false; + return; + } + + DVLOG(1) << "Set encoder color space to: " + << encoder_color_space_->ToString(); +} + } // namespace media diff --git a/media/gpu/mac/vt_video_encode_accelerator_mac.h b/media/gpu/mac/vt_video_encode_accelerator_mac.h index 66bb283094054..cbfd00a25b7fe 100644 --- a/media/gpu/mac/vt_video_encode_accelerator_mac.h +++ b/media/gpu/mac/vt_video_encode_accelerator_mac.h @@ -19,6 +19,7 @@ #include "media/gpu/media_gpu_export.h" #include "media/video/video_encode_accelerator.h" #include "third_party/webrtc/common_video/include/bitrate_adjuster.h" +#include "ui/gfx/color_space.h" namespace media { @@ -106,6 +107,8 @@ class MEDIA_GPU_EXPORT VTVideoEncodeAccelerator // encodes have been completed. void MaybeRunFlushCallback(); + void SetEncoderColorSpace(); + base::ScopedCFTypeRef compression_session_; gfx::Size input_visible_size_; @@ -153,6 +156,10 @@ class MEDIA_GPU_EXPORT VTVideoEncodeAccelerator int pending_encodes_ = 0; FlushCallback pending_flush_cb_; + // Color space of the first frame sent to Encode(). + absl::optional encoder_color_space_; + bool can_set_encoder_color_space_ = true; + // Declared last to ensure that all weak pointers are invalidated before // other destructors run. base::WeakPtr encoder_weak_ptr_; diff --git a/media/mojo/mojom/video_encode_accelerator.mojom b/media/mojo/mojom/video_encode_accelerator.mojom index 9dc7a8de7a32d..cc1ef9df69e88 100644 --- a/media/mojo/mojom/video_encode_accelerator.mojom +++ b/media/mojo/mojom/video_encode_accelerator.mojom @@ -9,6 +9,7 @@ import "media/mojo/mojom/media_types.mojom"; import "mojo/public/mojom/base/shared_memory.mojom"; import "mojo/public/mojom/base/time.mojom"; import "ui/gfx/geometry/mojom/geometry.mojom"; +import "ui/gfx/mojom/color_space.mojom"; import "media/mojo/mojom/video_encoder_info.mojom"; import "sandbox/policy/mojom/sandbox.mojom"; @@ -305,6 +306,7 @@ struct BitstreamBufferMetadata { int32 qp; CodecMetadata? codec_metadata; gfx.mojom.Size? encoded_size; + gfx.mojom.ColorSpace? encoded_color_space; }; interface VideoEncodeAcceleratorClient { diff --git a/media/mojo/mojom/video_encode_accelerator_mojom_traits.cc b/media/mojo/mojom/video_encode_accelerator_mojom_traits.cc index 341bd2717612c..5f30160c6b9c1 100644 --- a/media/mojo/mojom/video_encode_accelerator_mojom_traits.cc +++ b/media/mojo/mojom/video_encode_accelerator_mojom_traits.cc @@ -238,6 +238,9 @@ bool StructTraitsencoded_size)) { return false; } + if (!data.ReadEncodedColorSpace(&metadata->encoded_color_space)) { + return false; + } return data.ReadCodecMetadata(metadata); } diff --git a/media/mojo/mojom/video_encode_accelerator_mojom_traits.h b/media/mojo/mojom/video_encode_accelerator_mojom_traits.h index 784a4b745c430..35ea331bc780a 100644 --- a/media/mojo/mojom/video_encode_accelerator_mojom_traits.h +++ b/media/mojo/mojom/video_encode_accelerator_mojom_traits.h @@ -214,6 +214,10 @@ class StructTraits encoded_color_space( + const media::BitstreamBufferMetadata& bbm) { + return bbm.encoded_color_space; + } static bool Read(media::mojom::BitstreamBufferMetadataDataView data, media::BitstreamBufferMetadata* out_metadata); diff --git a/media/video/video_encode_accelerator.h b/media/video/video_encode_accelerator.h index 5dc25c566e44e..dedcf8848c9bb 100644 --- a/media/video/video_encode_accelerator.h +++ b/media/video/video_encode_accelerator.h @@ -22,6 +22,7 @@ #include "media/base/video_types.h" #include "media/video/video_encoder_info.h" #include "third_party/abseil-cpp/absl/types/optional.h" +#include "ui/gfx/color_space.h" namespace media { @@ -156,6 +157,9 @@ struct MEDIA_EXPORT BitstreamBufferMetadata final { // Some platforms may adjust the encoding size to meet hardware requirements. // If not set, the encoded size is the same as configured. absl::optional encoded_size; + + // Some platforms may adjust the color space. + absl::optional encoded_color_space; }; // Video encoder interface. diff --git a/media/video/video_encode_accelerator_adapter.cc b/media/video/video_encode_accelerator_adapter.cc index 72062cb77aa64..26bbebb046776 100644 --- a/media/video/video_encode_accelerator_adapter.cc +++ b/media/video/video_encode_accelerator_adapter.cc @@ -842,7 +842,8 @@ void VideoEncodeAcceleratorAdapter::BitstreamBufferReady( bool erased_active_encode = false; for (auto it = active_encodes_.begin(); it != active_encodes_.end(); ++it) { if ((*it)->timestamp == result.timestamp) { - result.color_space = (*it)->color_space; + result.color_space = + metadata.encoded_color_space.value_or((*it)->color_space); std::move((*it)->done_callback).Run(EncoderStatus::Codes::kOk); active_encodes_.erase(it); erased_active_encode = true; diff --git a/third_party/blink/renderer/modules/webcodecs/video_encoder.cc b/third_party/blink/renderer/modules/webcodecs/video_encoder.cc index 21bda31789cf0..dfc0569bb358b 100644 --- a/third_party/blink/renderer/modules/webcodecs/video_encoder.cc +++ b/third_party/blink/renderer/modules/webcodecs/video_encoder.cc @@ -816,15 +816,27 @@ bool VideoEncoder::StartReadback(scoped_refptr frame, ? viz::ResourceFormat::RGBA_8888 : viz::ResourceFormat::BGRA_8888; +#if BUILDFLAG(IS_APPLE) + // The Apple hardware encoder properly sets output color spaces, so we can + // round trip through the encoder and decoder w/o downgrading to BT.601. + constexpr auto kDstColorSpace = gfx::ColorSpace::CreateREC709(); +#else // When doing RGBA to YUVA conversion using `accelerated_frame_pool_`, use // sRGB primaries and the 601 YUV matrix. Note that this is subtly // different from the 601 gfx::ColorSpace because the 601 gfx::ColorSpace // has different (non-sRGB) primaries. - // https://crbug.com/1258245 + // + // This is necessary for our tests to pass since encoders will default to + // BT.601 when the color space information isn't told to the encoder. When + // coming back through the decoder it pulls out the embedded color space + // information instead of what's provided in the config. + // + // https://crbug.com/1258245, https://crbug.com/1377842 constexpr gfx::ColorSpace kDstColorSpace( gfx::ColorSpace::PrimaryID::BT709, gfx::ColorSpace::TransferID::SRGB, gfx::ColorSpace::MatrixID::SMPTE170M, gfx::ColorSpace::RangeID::LIMITED); +#endif TRACE_EVENT_NESTABLE_ASYNC_BEGIN1("media", "CopyRGBATextureToVideoFrame", this, "timestamp", frame->timestamp());