Treat color space changes as config changes.
It turns out some decoding pipelines don't reliably support just
evicting picture buffers. We need to actual go through a full
reset of the underlying decoder to avoid issues.
This removes the kColorSpaceChange constant and adds a feature
flag controlling the usage of color space changes (enabled by
default).
R=andrescj, sandersd
Bug: 1474191
Change-Id: I36e1e8beb02274776043d0ebc115d1048993102f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4919595
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Andres Calderon Jaramillo <andrescj@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1207936}
diff --git a/media/base/media_switches.cc b/media/base/media_switches.cc
index 9093d12..8a6d9c6 100644
--- a/media/base/media_switches.cc
+++ b/media/base/media_switches.cc
@@ -1099,6 +1099,11 @@
"AutoplayDisableSettings",
base::FEATURE_DISABLED_BY_DEFAULT);
+// Whether we should allow color space changes to flush AcceleratedVideoDecoder.
+BASE_FEATURE(kAVDColorSpaceChanges,
+ "AVDColorSpaceChanges",
+ base::FEATURE_ENABLED_BY_DEFAULT);
+
#if BUILDFLAG(IS_ANDROID)
// Should we allow video playback to use an overlay if it's not needed for
// security? Normally, we'd always want to allow this, except as part of the
diff --git a/media/base/media_switches.h b/media/base/media_switches.h
index 201bf82..4c45308 100644
--- a/media/base/media_switches.h
+++ b/media/base/media_switches.h
@@ -167,6 +167,7 @@
MEDIA_EXPORT BASE_DECLARE_FEATURE(kAutoPictureInPictureForVideoPlayback);
MEDIA_EXPORT BASE_DECLARE_FEATURE(kAutoplayIgnoreWebAudio);
MEDIA_EXPORT BASE_DECLARE_FEATURE(kAutoplayDisableSettings);
+MEDIA_EXPORT BASE_DECLARE_FEATURE(kAVDColorSpaceChanges);
#if !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_CHROMEOS)
MEDIA_EXPORT BASE_DECLARE_FEATURE(kCameraMicEffects);
diff --git a/media/gpu/accelerated_video_decoder.h b/media/gpu/accelerated_video_decoder.h
index 4b1ae46..e49b13bc 100644
--- a/media/gpu/accelerated_video_decoder.h
+++ b/media/gpu/accelerated_video_decoder.h
@@ -24,13 +24,12 @@
// frame and state management.
class MEDIA_GPU_EXPORT AcceleratedVideoDecoder {
public:
- AcceleratedVideoDecoder() {}
+ AcceleratedVideoDecoder() = default;
+ virtual ~AcceleratedVideoDecoder() = default;
AcceleratedVideoDecoder(const AcceleratedVideoDecoder&) = delete;
AcceleratedVideoDecoder& operator=(const AcceleratedVideoDecoder&) = delete;
- virtual ~AcceleratedVideoDecoder() {}
-
// Set the buffer owned by |decoder_buffer| as the current source of encoded
// stream data. AcceleratedVideoDecoder doesn't have an ownership of the
// buffer. |decoder_buffer| must be kept alive until Decode() returns
@@ -54,18 +53,10 @@
// in decoding; in future it could perhaps be possible to fall back
// to software decoding instead.
// kStreamError, // Error in stream.
- kConfigChange, // This is returned when some configuration (e.g.
- // profile or picture size) is changed. A client may
- // need to apply the client side the configuration
- // properly (e.g. allocate buffers with the new
- // resolution).
- kColorSpaceChange, // This is returned if the video color space is changed.
- // Color space changes off of key frames are discarded
- // only for VP9 decoder. When both ConfigChange and
- // ColorSpaceChange occur together, ConfigChange is
- // preferred over ColorSpaceChange. When triggered, it
- // is used for creating new shared images for the
- // D3D11VideoDecoder.
+ kConfigChange, // Stream configuration has changed. E.g., profile, coded
+ // size, bit depth, or color space. A client may need to
+ // reallocate resources to apply the new configuration
+ // properly. E.g. allocate buffers with the new resolution.
kRanOutOfStreamData, // Need more stream data to proceed.
kRanOutOfSurfaces, // Waiting for the client to free up output surfaces.
kNeedContextUpdate, // Waiting for the client to update decoding context
diff --git a/media/gpu/av1_decoder.cc b/media/gpu/av1_decoder.cc
index 192b2a9..5145025 100644
--- a/media/gpu/av1_decoder.cc
+++ b/media/gpu/av1_decoder.cc
@@ -11,6 +11,7 @@
#include "base/memory/ptr_util.h"
#include "base/ranges/algorithm.h"
#include "media/base/limits.h"
+#include "media/base/media_switches.h"
#include "media/gpu/av1_picture.h"
#include "third_party/libgav1/src/src/decoder_state.h"
#include "third_party/libgav1/src/src/gav1/status_code.h"
@@ -321,12 +322,24 @@
new_color_space = container_color_space_;
}
+ bool is_color_space_change = false;
+ if (base::FeatureList::IsEnabled(kAVDColorSpaceChanges)) {
+ is_color_space_change = new_color_space.IsSpecified() &&
+ new_color_space != picture_color_space_;
+ }
+
ClearReferenceFrames();
// Issues kConfigChange only if either the dimensions, profile or bit
// depth is changed.
if (frame_size_ != new_frame_size ||
visible_rect_ != new_visible_rect || profile_ != new_profile ||
- bit_depth_ != new_bit_depth) {
+ bit_depth_ != new_bit_depth || is_color_space_change) {
+ DVLOG(1) << "New profile: " << GetProfileName(new_profile)
+ << ", new resolution: " << new_frame_size.ToString()
+ << ", new visible rect: " << new_visible_rect.ToString()
+ << ", new bit depth: "
+ << base::strict_cast<int>(new_bit_depth)
+ << ", new color space: " << new_color_space.ToString();
frame_size_ = new_frame_size;
visible_rect_ = new_visible_rect;
profile_ = new_profile;
@@ -335,14 +348,6 @@
clear_current_frame.ReplaceClosure(base::DoNothing());
return kConfigChange;
}
-
- // Trigger color space change if the previous picture color space is
- // different from new color space.
- if (new_color_space.IsSpecified() &&
- picture_color_space_ != new_color_space) {
- picture_color_space_ = new_color_space;
- return kColorSpaceChange;
- }
}
}
diff --git a/media/gpu/h264_decoder.cc b/media/gpu/h264_decoder.cc
index e3ca4f6c..f408c3d 100644
--- a/media/gpu/h264_decoder.cc
+++ b/media/gpu/h264_decoder.cc
@@ -1118,9 +1118,7 @@
return true;
}
-bool H264Decoder::ProcessSPS(int sps_id,
- bool* need_new_buffers,
- bool* color_space_changed) {
+bool H264Decoder::ProcessSPS(int sps_id, bool* need_new_buffers) {
DVLOG(4) << "Processing SPS id:" << sps_id;
const H264SPS* sps = parser_.GetSPS(sps_id);
@@ -1217,15 +1215,24 @@
new_color_space = container_color_space_;
}
+ bool is_color_space_change = false;
+ if (base::FeatureList::IsEnabled(kAVDColorSpaceChanges)) {
+ is_color_space_change = new_color_space.IsSpecified() &&
+ new_color_space != picture_color_space_;
+ }
+
if (pic_size_ != new_pic_size || dpb_.max_num_pics() != max_dpb_size ||
- profile_ != new_profile || bit_depth_ != new_bit_depth) {
- if (!Flush())
+ profile_ != new_profile || bit_depth_ != new_bit_depth ||
+ is_color_space_change) {
+ if (!Flush()) {
return false;
+ }
DVLOG(1) << "Codec profile: " << GetProfileName(new_profile)
<< ", level: " << base::strict_cast<int>(level)
<< ", DPB size: " << max_dpb_size
<< ", Picture size: " << new_pic_size.ToString()
- << ", bit depth: " << base::strict_cast<int>(new_bit_depth);
+ << ", bit depth: " << base::strict_cast<int>(new_bit_depth)
+ << ", color_space: " << new_color_space.ToString();
*need_new_buffers = true;
profile_ = new_profile;
bit_depth_ = new_bit_depth;
@@ -1234,18 +1241,6 @@
dpb_.set_max_num_pics(max_dpb_size);
}
- // If the new color space is specified and different from picture color space
- // then trigger color space change.
- if (new_color_space.IsSpecified() &&
- new_color_space != picture_color_space_) {
- if (!Flush()) {
- return false;
- }
- DVLOG(1) << "New color space: " << new_color_space.ToString();
- picture_color_space_ = new_color_space;
- *color_space_changed = true;
- }
-
gfx::Rect new_visible_rect = sps->GetVisibleRect().value_or(gfx::Rect());
if (visible_rect_ != new_visible_rect) {
DVLOG(2) << "New visible rect: " << new_visible_rect.ToString();
@@ -1602,8 +1597,7 @@
SET_ERROR_AND_RETURN();
bool need_new_buffers = false;
- bool color_space_changed = false;
- if (!ProcessSPS(sps_id, &need_new_buffers, &color_space_changed)) {
+ if (!ProcessSPS(sps_id, &need_new_buffers)) {
SET_ERROR_AND_RETURN();
}
accelerator_->ProcessSPS(
@@ -1615,7 +1609,7 @@
if (state_ == State::kNeedStreamMetadata)
state_ = State::kAfterReset;
- if (need_new_buffers || color_space_changed) {
+ if (need_new_buffers) {
curr_pic_ = nullptr;
curr_nalu_ = nullptr;
ref_pic_list_p0_.clear();
@@ -1626,9 +1620,6 @@
if (need_new_buffers) {
return kConfigChange;
}
- if (color_space_changed) {
- return kColorSpaceChange;
- }
break;
}
diff --git a/media/gpu/h264_decoder.h b/media/gpu/h264_decoder.h
index 039d265..9a37813 100644
--- a/media/gpu/h264_decoder.h
+++ b/media/gpu/h264_decoder.h
@@ -239,9 +239,7 @@
};
// Process H264 stream structures.
- bool ProcessSPS(int sps_id,
- bool* need_new_buffers,
- bool* color_space_changed);
+ bool ProcessSPS(int sps_id, bool* need_new_buffers);
// Processes a CENCv1 encrypted slice header and fills in |curr_slice_hdr_|
// with the relevant parsed fields.
diff --git a/media/gpu/h265_decoder.cc b/media/gpu/h265_decoder.cc
index c88402e..a2aaab2 100644
--- a/media/gpu/h265_decoder.cc
+++ b/media/gpu/h265_decoder.cc
@@ -7,6 +7,7 @@
#include "base/logging.h"
#include "base/notreached.h"
#include "media/base/limits.h"
+#include "media/base/media_switches.h"
#include "media/gpu/h265_decoder.h"
namespace media {
@@ -359,9 +360,8 @@
state_ = kTryPreprocessCurrentSlice;
if (curr_slice_hdr_->irap_pic) {
bool need_new_buffers = false;
- bool color_space_changed = false;
if (!ProcessPPS(curr_slice_hdr_->slice_pic_parameter_set_id,
- &need_new_buffers, &color_space_changed)) {
+ &need_new_buffers)) {
SET_ERROR_AND_RETURN();
}
@@ -369,10 +369,6 @@
curr_pic_ = nullptr;
return kConfigChange;
}
- if (color_space_changed) {
- curr_pic_ = nullptr;
- return kColorSpaceChange;
- }
}
}
@@ -457,8 +453,7 @@
// active stream.
if (curr_pps_id_ == -1) {
bool need_new_buffers = false;
- bool color_space_changed = false;
- if (!ProcessPPS(pps_id, &need_new_buffers, &color_space_changed)) {
+ if (!ProcessPPS(pps_id, &need_new_buffers)) {
SET_ERROR_AND_RETURN();
}
@@ -466,10 +461,6 @@
curr_nalu_.reset();
return kConfigChange;
}
- if (color_space_changed) {
- curr_nalu_.reset();
- return kColorSpaceChange;
- }
}
break;
@@ -571,9 +562,7 @@
return dpb_.max_num_pics();
}
-bool H265Decoder::ProcessPPS(int pps_id,
- bool* need_new_buffers,
- bool* color_space_changed) {
+bool H265Decoder::ProcessPPS(int pps_id, bool* need_new_buffers) {
DVLOG(4) << "Processing PPS id:" << pps_id;
const H265PPS* pps = parser_.GetPPS(pps_id);
@@ -587,10 +576,6 @@
if (need_new_buffers)
*need_new_buffers = false;
- if (color_space_changed) {
- *color_space_changed = false;
- }
-
gfx::Size new_pic_size = sps->GetCodedSize();
gfx::Rect new_visible_rect = sps->GetVisibleRect();
if (visible_rect_ != new_visible_rect) {
@@ -632,9 +617,15 @@
new_color_space = container_color_space_;
}
+ bool is_color_space_change = false;
+ if (base::FeatureList::IsEnabled(kAVDColorSpaceChanges)) {
+ is_color_space_change = new_color_space.IsSpecified() &&
+ new_color_space != picture_color_space_;
+ }
+
if (pic_size_ != new_pic_size || dpb_.max_num_pics() != sps->max_dpb_size ||
profile_ != new_profile || bit_depth_ != new_bit_depth ||
- chroma_sampling_ != new_chroma_sampling) {
+ chroma_sampling_ != new_chroma_sampling || is_color_space_change) {
if (!Flush())
return false;
DVLOG(1) << "Codec profile: " << GetProfileName(new_profile)
@@ -654,16 +645,6 @@
*need_new_buffers = true;
}
- if (new_color_space.IsSpecified() &&
- new_color_space != picture_color_space_) {
- if (!Flush()) {
- return false;
- }
- DVLOG(1) << "Picture color space: " << new_color_space.ToString();
- picture_color_space_ = new_color_space;
- *color_space_changed = true;
- }
-
return true;
}
diff --git a/media/gpu/h265_decoder.h b/media/gpu/h265_decoder.h
index cc904a77..d50ff58 100644
--- a/media/gpu/h265_decoder.h
+++ b/media/gpu/h265_decoder.h
@@ -226,9 +226,7 @@
};
// Process H265 stream structures.
- bool ProcessPPS(int pps_id,
- bool* need_new_buffers,
- bool* color_space_changed);
+ bool ProcessPPS(int pps_id, bool* need_new_buffers);
// Process current slice header to discover if we need to start a new picture,
// finishing up the current one.
diff --git a/media/gpu/mac/video_toolbox_video_decoder.cc b/media/gpu/mac/video_toolbox_video_decoder.cc
index 4e38b4b..70e6420c 100644
--- a/media/gpu/mac/video_toolbox_video_decoder.cc
+++ b/media/gpu/mac/video_toolbox_video_decoder.cc
@@ -265,7 +265,6 @@
return;
case AcceleratedVideoDecoder::kConfigChange:
- case AcceleratedVideoDecoder::kColorSpaceChange:
continue;
case AcceleratedVideoDecoder::kRanOutOfStreamData:
diff --git a/media/gpu/v4l2/stateless/v4l2_stateless_video_decoder.cc b/media/gpu/v4l2/stateless/v4l2_stateless_video_decoder.cc
index 9f35c6f..adf56cbe7 100644
--- a/media/gpu/v4l2/stateless/v4l2_stateless_video_decoder.cc
+++ b/media/gpu/v4l2/stateless/v4l2_stateless_video_decoder.cc
@@ -243,10 +243,6 @@
<< " of resolution " << decoder_->GetPicSize().ToString();
}
break;
- case AcceleratedVideoDecoder::kColorSpaceChange:
- VLOGF(2) << "AcceleratedVideoDecoder::kColorSpaceChange";
- NOTIMPLEMENTED();
- break;
case AcceleratedVideoDecoder::kRanOutOfStreamData:
VLOGF(2) << "AcceleratedVideoDecoder::kRanOutOfStreamData";
// Handled on first entry to function.
diff --git a/media/gpu/v4l2/v4l2_video_decoder_backend_stateless.cc b/media/gpu/v4l2/v4l2_video_decoder_backend_stateless.cc
index dd2eda3..3876c62 100644
--- a/media/gpu/v4l2/v4l2_video_decoder_backend_stateless.cc
+++ b/media/gpu/v4l2/v4l2_video_decoder_backend_stateless.cc
@@ -439,10 +439,6 @@
PumpOutputSurfaces();
return true;
- case AcceleratedVideoDecoder::kColorSpaceChange:
- NOTIMPLEMENTED_LOG_ONCE();
- return false;
-
case AcceleratedVideoDecoder::kRanOutOfStreamData:
// Current decode request is finished processing.
if (current_decode_request_) {
diff --git a/media/gpu/vaapi/vaapi_video_decoder.cc b/media/gpu/vaapi/vaapi_video_decoder.cc
index 0e0f902..112b825 100644
--- a/media/gpu/vaapi/vaapi_video_decoder.cc
+++ b/media/gpu/vaapi/vaapi_video_decoder.cc
@@ -431,9 +431,6 @@
SetState(State::kChangingResolution);
client_->PrepareChangeResolution();
break;
- case AcceleratedVideoDecoder::kColorSpaceChange:
- NOTIMPLEMENTED_LOG_ONCE();
- break;
case AcceleratedVideoDecoder::kRanOutOfSurfaces:
// No more surfaces to decode into available, wait until client returns
// video frames to the frame pool.
diff --git a/media/gpu/vp9_decoder.cc b/media/gpu/vp9_decoder.cc
index 8dfb1a85..9f71d15 100644
--- a/media/gpu/vp9_decoder.cc
+++ b/media/gpu/vp9_decoder.cc
@@ -291,6 +291,12 @@
DCHECK(!new_pic_size.IsEmpty());
+ bool is_color_space_change = false;
+ if (base::FeatureList::IsEnabled(kAVDColorSpaceChanges)) {
+ is_color_space_change = new_color_space.IsSpecified() &&
+ new_color_space != picture_color_space_;
+ }
+
const bool is_pic_size_different = new_pic_size != pic_size_;
const bool is_pic_size_larger = new_pic_size.width() > pic_size_.width() ||
new_pic_size.height() > pic_size_.height();
@@ -298,13 +304,15 @@
(ignore_resolution_changes_to_smaller_for_testing_
? is_pic_size_larger
: is_pic_size_different) ||
- new_profile != profile_ || curr_frame_hdr_->bit_depth != bit_depth_;
+ new_profile != profile_ || curr_frame_hdr_->bit_depth != bit_depth_ ||
+ is_color_space_change;
if (is_new_configuration_different_enough) {
DVLOG(1) << "New profile: " << GetProfileName(new_profile)
<< ", new resolution: " << new_pic_size.ToString()
<< ", new bit depth: "
- << base::strict_cast<int>(curr_frame_hdr_->bit_depth);
+ << base::strict_cast<int>(curr_frame_hdr_->bit_depth)
+ << ", new color space: " << new_color_space.ToString();
if (!curr_frame_hdr_->IsKeyframe() &&
!(curr_frame_hdr_->IsIntra() && pic_size_.IsEmpty())) {
@@ -339,19 +347,6 @@
return kConfigChange;
}
- if (new_color_space.IsSpecified() &&
- new_color_space != picture_color_space_ &&
- curr_frame_hdr_->IsKeyframe()) {
- // TODO(posciak): This requires us to be on a keyframe (see above) and is
- // required, because VDA clients expect all surfaces to be returned before
- // they can cycle surface sets after receiving kConfigChange.
- // This is only an implementation detail of VDAs and can be improved.
- ref_frames_.Clear();
-
- picture_color_space_ = new_color_space;
- return kColorSpaceChange;
- }
-
scoped_refptr<VP9Picture> pic = accelerator_->CreateVP9Picture();
if (!pic) {
return kRanOutOfSurfaces;
diff --git a/media/gpu/windows/d3d11_video_decoder.cc b/media/gpu/windows/d3d11_video_decoder.cc
index 7528ca2..3bd3187 100644
--- a/media/gpu/windows/d3d11_video_decoder.cc
+++ b/media/gpu/windows/d3d11_video_decoder.cc
@@ -643,24 +643,29 @@
const auto new_coded_size = accelerated_video_decoder_->GetPicSize();
const auto new_chroma_sampling =
accelerated_video_decoder_->GetChromaSampling();
+ const auto new_color_space =
+ accelerated_video_decoder_->GetVideoColorSpace();
if (new_profile == config_.profile() &&
new_coded_size == config_.coded_size() &&
new_bit_depth == bit_depth_ && !picture_buffers_.size() &&
- new_chroma_sampling == chroma_sampling_) {
+ new_chroma_sampling == chroma_sampling_ &&
+ new_color_space == color_space_) {
continue;
}
// Update the config.
MEDIA_LOG(INFO, media_log_)
<< "D3D11VideoDecoder config change: profile: "
- << static_cast<int>(new_profile) << " chroma_sampling_format: "
+ << GetProfileName(new_profile) << ", chroma_sampling_format: "
<< VideoChromaSamplingToString(new_chroma_sampling)
- << " coded_size: (" << new_coded_size.width() << ", "
- << new_coded_size.height() << ")";
+ << ", coded_size: " << new_coded_size.ToString()
+ << ", bit_depth: " << base::strict_cast<int>(new_bit_depth)
+ << ", color_space: " << new_color_space.ToString();
profile_ = new_profile;
config_.set_profile(profile_);
config_.set_coded_size(new_coded_size);
chroma_sampling_ = new_chroma_sampling;
+ color_space_ = new_color_space;
// Replace the decoder, and clear any picture buffers we have. It's okay
// if we don't have any picture buffer yet; this might be before the
@@ -677,14 +682,6 @@
CHECK(set_accelerator_decoder_wrapper_cb_);
set_accelerator_decoder_wrapper_cb_.Run(std::move(wrapper));
picture_buffers_.clear();
- } else if (result == media::AcceleratedVideoDecoder::kColorSpaceChange) {
- MEDIA_LOG(INFO, media_log_)
- << "D3D11VideoDecoder color space change: color_space: "
- << accelerated_video_decoder_->GetVideoColorSpace().ToString();
-
- // Clear the picture buffers and recreate the pictures leading to new
- // shared images with new color space.
- picture_buffers_.clear();
} else if (result == media::AcceleratedVideoDecoder::kTryAgain) {
LOG(ERROR) << "Try again is not supported";
NotifyError(D3D11Status::Codes::kTryAgainNotSupported);
diff --git a/media/gpu/windows/d3d11_video_decoder.h b/media/gpu/windows/d3d11_video_decoder.h
index 8d5d14d..f0ded91 100644
--- a/media/gpu/windows/d3d11_video_decoder.h
+++ b/media/gpu/windows/d3d11_video_decoder.h
@@ -289,6 +289,10 @@
// need to recreate the decoder.
uint8_t bit_depth_ = 8u;
+ // The currently configured color space for the decoder. When this changes we
+ // need to recreate the decoder.
+ VideoColorSpace color_space_;
+
// The currently configured chroma sampling format on the accelerator. When
// this changes we need to recreate the decoder.
VideoChromaSampling chroma_sampling_ = VideoChromaSampling::k420;