Commit 3860259f authored by Wan-Teh Chang's avatar Wan-Teh Chang Committed by Commit Bot

Fix a bug in IsColorSpaceSupportedByPCVR()

This bug was introduced in https://crrev.com/c/2321307. It was detected
by the new Web platform test in my pending CL
https://crrev.com/c/2304961.

The following return statement in IsColorSpaceSupportedByPCVR()

  if (!image->alphaPlane)
    return true;

actually elided a check that yuv_color_space was one of the four
SkYUVColorSpace values supported by media::PaintCanvasVideoRenderer
(PCVR). At that time (before
https://skia-review.googlesource.com/c/skia/+/305596) SkYUVColorSpace
had only those four values (except for kIdentity_SkYUVColorSpace) and
our own GetSkYUVColorSpace() function returned only those four values on
success, so it was fine to elide that check. After we switched from our
own GetSkYUVColorSpace() function to
gfx::ColorSpace::ToSkYUVColorSpace() in https://crrev.com/c/2321307, the
newly added SkYUVColorSpace values may be returned, so it is no longer
fine to elide the check.

Also make two cleanup changes:

1. Replace the deprecated SkYUVColorSpace enumerators with the new more
verbose ones.

2. Do not declare the yuv_color_space_ member as base::Optional.
base::Optional made sense because our own GetSkYUVColorSpace() function
returned base::Optional instead of using an output parameter. Since
gfx::ColorSpace::ToSkYUVColorSpace() uses an output parameter, it is
cumbersome to pass a base::Optional as an output parameter.

Bug: 1108626
Change-Id: I96dba5c54dbe1b826be714141665adcaee47bccb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2343535Reviewed-by: default avatarPeter Kasting <pkasting@chromium.org>
Commit-Queue: Wan-Teh Chang <wtc@google.com>
Cr-Commit-Position: refs/heads/master@{#796150}
parent ffdc6dd5
...@@ -104,12 +104,16 @@ bool IsColorSpaceSupportedByPCVR(const avifImage* image) { ...@@ -104,12 +104,16 @@ bool IsColorSpaceSupportedByPCVR(const avifImage* image) {
SkYUVColorSpace yuv_color_space; SkYUVColorSpace yuv_color_space;
if (!GetColorSpace(image).ToSkYUVColorSpace(image->depth, &yuv_color_space)) if (!GetColorSpace(image).ToSkYUVColorSpace(image->depth, &yuv_color_space))
return false; return false;
if (!image->alphaPlane) if (!image->alphaPlane) {
return true; return yuv_color_space == kJPEG_Full_SkYUVColorSpace ||
yuv_color_space == kRec601_Limited_SkYUVColorSpace ||
yuv_color_space == kRec709_Limited_SkYUVColorSpace ||
yuv_color_space == kBT2020_8bit_Limited_SkYUVColorSpace;
}
// libyuv supports the alpha channel only with the I420 pixel format, which is // libyuv supports the alpha channel only with the I420 pixel format, which is
// 8-bit YUV 4:2:0 with kRec601_SkYUVColorSpace. // 8-bit YUV 4:2:0 with kRec601_Limited_SkYUVColorSpace.
return image->depth == 8 && image->yuvFormat == AVIF_PIXEL_FORMAT_YUV420 && return image->depth == 8 && image->yuvFormat == AVIF_PIXEL_FORMAT_YUV420 &&
yuv_color_space == kRec601_SkYUVColorSpace && yuv_color_space == kRec601_Limited_SkYUVColorSpace &&
image->alphaRange == AVIF_RANGE_FULL; image->alphaRange == AVIF_RANGE_FULL;
} }
...@@ -315,8 +319,8 @@ size_t AVIFImageDecoder::DecodedYUVWidthBytes(int component) const { ...@@ -315,8 +319,8 @@ size_t AVIFImageDecoder::DecodedYUVWidthBytes(int component) const {
SkYUVColorSpace AVIFImageDecoder::GetYUVColorSpace() const { SkYUVColorSpace AVIFImageDecoder::GetYUVColorSpace() const {
DCHECK(CanDecodeToYUV()); DCHECK(CanDecodeToYUV());
DCHECK(yuv_color_space_); DCHECK_NE(yuv_color_space_, SkYUVColorSpace::kIdentity_SkYUVColorSpace);
return *yuv_color_space_; return yuv_color_space_;
} }
uint8_t AVIFImageDecoder::GetYUVBitDepth() const { uint8_t AVIFImageDecoder::GetYUVBitDepth() const {
...@@ -697,7 +701,7 @@ bool AVIFImageDecoder::MaybeCreateDemuxer() { ...@@ -697,7 +701,7 @@ bool AVIFImageDecoder::MaybeCreateDemuxer() {
allow_decode_to_yuv_ = avif_yuv_format_ != AVIF_PIXEL_FORMAT_YUV400 && allow_decode_to_yuv_ = avif_yuv_format_ != AVIF_PIXEL_FORMAT_YUV400 &&
!decoder_->alphaPresent && decoded_frame_count_ == 1 && !decoder_->alphaPresent && decoded_frame_count_ == 1 &&
GetColorSpace(container).ToSkYUVColorSpace( GetColorSpace(container).ToSkYUVColorSpace(
container->depth, &yuv_color_space_.emplace()) && container->depth, &yuv_color_space_) &&
!ColorTransform(); !ColorTransform();
return SetSize(container->width, container->height); return SetSize(container->width, container->height);
} }
......
...@@ -7,7 +7,6 @@ ...@@ -7,7 +7,6 @@
#include <memory> #include <memory>
#include "base/optional.h"
#include "third_party/blink/renderer/platform/image-decoders/image_decoder.h" #include "third_party/blink/renderer/platform/image-decoders/image_decoder.h"
#include "third_party/skia/include/core/SkBitmap.h" #include "third_party/skia/include/core/SkBitmap.h"
#include "third_party/skia/include/core/SkData.h" #include "third_party/skia/include/core/SkData.h"
...@@ -87,7 +86,7 @@ class PLATFORM_EXPORT AVIFImageDecoder final : public ImageDecoder { ...@@ -87,7 +86,7 @@ class PLATFORM_EXPORT AVIFImageDecoder final : public ImageDecoder {
uint8_t chroma_shift_x_ = 0; uint8_t chroma_shift_x_ = 0;
uint8_t chroma_shift_y_ = 0; uint8_t chroma_shift_y_ = 0;
size_t decoded_frame_count_ = 0; size_t decoded_frame_count_ = 0;
base::Optional<SkYUVColorSpace> yuv_color_space_; SkYUVColorSpace yuv_color_space_ = SkYUVColorSpace::kIdentity_SkYUVColorSpace;
std::unique_ptr<avifDecoder, void (*)(avifDecoder*)> decoder_{nullptr, std::unique_ptr<avifDecoder, void (*)(avifDecoder*)> decoder_{nullptr,
nullptr}; nullptr};
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment