Skip to content

Commit

Permalink
Revert "Reland: "Update macOS VideoToolbox encoder color space settin…
Browse files Browse the repository at this point in the history 
…gs.""

This reverts commit e6e93d8.

Reason for revert: Breaks MacOS bots:
https://ci.chromium.org/ui/p/chromium/builders/ci/Mac%20Retina%20Debug%20(AMD)/117129
https://ci.chromium.org/ui/p/chromium/builders/ci/Mac%20Retina%20Release%20(AMD)/160606
These tests are failing:
Pixel_WebGPUImportVideoFrame
Pixel_WebGPUImportVideoFrameOffscreenCanvas

Original change's description:
> 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 <ajgo@chromium.org>
> Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
> Reviewed-by: Eugene Zemtsov <eugene@chromium.org>
> Reviewed-by: Brian Sheedy <bsheedy@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1117755}

Bug: 1377842
Change-Id: I2d84284c26cd06fdbc186926886ae7cd477cf5ec
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
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4342177
Auto-Submit: Alexis Hétu <sugoi@chromium.org>
Commit-Queue: Alexis Hétu <sugoi@chromium.org>
Owners-Override: Alexis Hétu <sugoi@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1117814}
  • Loading branch information
Alexis Hétu authored and Chromium LUCI CQ committed Mar 15, 2023
1 parent 3be3bec commit 67ccd66
Show file tree
Hide file tree
Showing 15 changed files with 113 additions and 431 deletions.
17 changes: 3 additions & 14 deletions content/test/data/gpu/webcodecs/copyTo.html
View file
Expand Up @@ -7,20 +7,13 @@
<script type="text/javascript">
'use strict';

function yuv_to_rgb601(y, u, v) {
function yuv_to_rgb(y, u, v) {
let b = 1.164 * (y - 16) + 2.018 * (u - 128);
let g = 1.164 * (y - 16) - 0.813 * (v - 128) - 0.391 * (u - 128);
let r = 1.164 * (y - 16) + 1.596 * (v - 128);
return { r: r, g: g, b: b };
}

function yuv_to_rgb709(y, u, v) {
let b = 1.164 * (y - 16) + 2.113 * (u - 128);
let g = 1.164 * (y - 16) - 0.533 * (v - 128) - 0.213 * (u - 128);
let r = 1.164 * (y - 16) + 1.793 * (v - 128);
return { r: r, g: g, b: b };
}

async function validateFourColorsBytes(frame) {
const m = 4;
const tolerance = 8;
Expand All @@ -46,16 +39,12 @@
let view = new DataView(buffer);
let rgb = null;

let yuv_to_rgb = yuv_to_rgb601;
if (frame.colorSpace.matrix == "bt709")
yuv_to_rgb = yuv_to_rgb709;

switch (frame.format) {
case "I420":
case "I420A":
rgb = yuv_to_rgb(view.getUint8(layout[0].offset), // Y
view.getUint8(layout[1].offset), // U
view.getUint8(layout[2].offset)); // V
view.getUint8(layout[1].offset), // U
view.getUint8(layout[2].offset)); // V
break;
case "NV12":
rgb = yuv_to_rgb(view.getUint8(layout[0].offset), // Y
Expand Down
194 changes: 56 additions & 138 deletions content/test/data/gpu/webcodecs/encode-color-space.html
View file
@@ -1,12 +1,12 @@
<!DOCTYPE html>
<!--
Encodes two I420 frames and checks that output color space is changed to Rec709.
Then encodes two RGBA frame w/ Rec601 colors and checks that the output color
space is changed to Rec601, and that a key frame is generated alongside the
change. It then verifies the decoded bitstream has the proper color information.
Encodes two RGB frames and checks that output color space is changed to Rec601.
Then encodes two I420 frame w/ Rec709 colors and checks that the output color
space is changed to Rec709, and that a key frame is generated alongside the
change.
The spec doesn't mandate any particular output color space, so this just checks
that our internal behavior is surfaced correctly. A key frame is expected when
that our internal behvaior is surfaced correctly. A key frame is expected when
color space changes to mirror the our decoder requirement to provide a key frame
whenever VideoDecoderConfig changes.
-->
Expand All @@ -26,22 +26,24 @@
const FRAME_HEIGHT = 480;

function isRec709(colorSpace) {
return colorSpace.primaries === 'bt709' &&
colorSpace.transfer === 'bt709' && colorSpace.matrix === 'bt709' &&
colorSpace.fullRange === false;
return colorSpace.primaries === 'bt709'
&& colorSpace.transfer === 'bt709'
&& colorSpace.matrix === 'bt709'
&& colorSpace.fullRange === false;
}

function isSRGB(colorSpace) {
return colorSpace.primaries === 'bt709' &&
colorSpace.transfer === 'iec61966-2-1' &&
colorSpace.matrix === 'rgb' && colorSpace.fullRange === true;
return colorSpace.primaries === 'bt709'
&& colorSpace.transfer === 'iec61966-2-1'
&& colorSpace.matrix === 'rgb'
&& colorSpace.fullRange === true;
}

function isRec601(colorSpace) {
return colorSpace.primaries === 'smpte170m' &&
(colorSpace.transfer === 'smpte170m' ||
colorSpace.transfer === 'bt709') &&
colorSpace.matrix === 'smpte170m' && colorSpace.fullRange === false;
return colorSpace.primaries === 'smpte170m'
&& colorSpace.transfer === 'smpte170m'
&& colorSpace.matrix === 'smpte170m'
&& colorSpace.fullRange === false;
}

function makePixelArray(byteLength) {
Expand Down Expand Up @@ -88,21 +90,18 @@
}

const frameDuration = 16666;
let inputFrames = [
// Use I420/BT.709 first since default macOS colorspace is sRGB.
makeFrame('I420', 0 * frameDuration),
makeFrame('I420', 1 * frameDuration),
makeFrame('RGBA', 2 * frameDuration),
makeFrame('RGBA', 3 * frameDuration),
];
let inputFrames = [makeFrame('RGBA', 0 * frameDuration),
makeFrame('RGBA', 1 * frameDuration),
makeFrame('I420', 2 * frameDuration),
makeFrame('I420', 3 * frameDuration)];
let outputChunks = [];
let outputMetadata = [];
let outputMetadatas = [];
let errors = 0;

const init = {
output(chunk, metadata) {
outputChunks.push(chunk);
outputMetadata.push(metadata);
outputMetadatas.push(metadata);
},
error(e) {
errors++;
Expand All @@ -120,129 +119,48 @@
await encoder.flush();
encoder.close();

TEST.assert_eq(errors, 0, 'Encoding errors occurred during the test');
TEST.assert_eq(outputChunks.length, 4, 'Unexpected number of outputs');
TEST.assert_eq(
outputMetadata.length, 4, 'Unexpected number of output metadata');

// I420 passthrough should preserve default rec709 color space.
TEST.assert_eq(inputFrames[0].format, 'I420', 'inputs[0] is I420');
TEST.assert(isRec709(inputFrames[0].colorSpace), 'inputs[0] is rec709');
TEST.assert_eq(outputChunks[0].type, 'key', 'outputs[0] is key');
TEST.assert(
'decoderConfig' in outputMetadata[0],
'metadata[0] has decoderConfig');
TEST.assert(
isRec709(outputMetadata[0].decoderConfig.colorSpace),
'metadata[0] is rec709');

// Next output may or may not be a key frame w/ metadata (up to
// encoder). Corresponding input is still I420 rec709, so if metadata is
TEST.assert(errors == 0, 'Encoding errors occurred during the test');
TEST.assert(outputChunks.length == 4, 'Unexpected number of outputs');
TEST.assert(outputMetadatas.length == 4, 'Unexpected number of output metadatas');

// First output should be a key frame and have acompanying metadata.
// Corresponding input RGBA sRGB, so we expect encoder will have used
// libYUV to convert to I420 or NV12 rec601.
TEST.assert(inputFrames[0].format == 'RGBA', 'inputs[0] is RGBA');
TEST.assert(isSRGB(inputFrames[0].colorSpace), 'inputs[0] is sRGB');
TEST.assert(outputChunks[0].type == 'key', 'outputs[0] is key');
TEST.assert('decoderConfig' in outputMetadatas[0], 'metdatas[0] has decoderConfig');
TEST.assert(isRec601(outputMetadatas[0].decoderConfig.colorSpace), 'metdatas[0] is rec601');

// Second outupt may or may not be a key frame w/ metadata (up to
// encoder). Corresponding input is still RGBA sRGB, so if metadata is
// given, we expect same colorSpace as for the previous frame.
TEST.assert_eq(inputFrames[1].format, 'I420', 'inputs[1] is I420');
TEST.assert(isRec709(inputFrames[1].colorSpace, 'inputs[1] is rec709'));
if ('decoderConfig' in outputMetadata[1]) {
TEST.assert(
isRec709(outputMetadata[1].decoderConfig.colorSpace),
'metadata[1] is rec709');
TEST.assert(inputFrames[1].format == 'RGBA', 'inputs[1] is RGBA');
TEST.assert(isSRGB(inputFrames[1].colorSpace), 'inputs[1] is sRGB');
if('decoderConfig' in outputMetadatas[1]) {
TEST.assert(isRec601(outputMetadatas[1].decoderConfig.colorSpace), 'metdatas[1] is rec601');
}

// Next output should be a key frame and have accompanying metadata
// because the corresponding input format changed to RGBA, which means
// we libyuv will convert to I420 w/ rec601 during encoding.
TEST.assert_eq(inputFrames[2].format, 'RGBA', 'inputs[2] is RGBA');
TEST.assert(isSRGB(inputFrames[2].colorSpace), 'inputs[2] is sRGB');

// Third output should be a key frame and have acompanying metadata
// because the correpsonding input format changed to I420, which means
// we passthrough without libYUV conversion and a new decoderConfig
// should reflect that.
TEST.assert(inputFrames[2].format == 'I420', 'inputs[2] is I420');
TEST.assert(isRec709(inputFrames[2].colorSpace), 'inputs[2] is rec709');
// TODO(https://crbug.com/1241448): Uncomment the line below once android
// reliably produces a key frame at the appropriate time. For now this
// is covered by a DCHECK (excluding android) in video_encoder.cc.
// TEST.assert(outputChunks[2].type == 'key', 'outputs[2] is key');
TEST.assert('decoderConfig' in outputMetadatas[2], 'metadatas[2] has decoderConfig');
TEST.assert(isRec709(outputMetadatas[2].decoderConfig.colorSpace), 'metadatas[2] is rec709');

TEST.assert(
'decoderConfig' in outputMetadata[2],
'metadata[2] has decoderConfig');
TEST.assert(
isRec601(outputMetadata[2].decoderConfig.colorSpace),
'metadata[2] is rec601');

// Next output may or may not be a key frame w/ metadata (up to
// encoder). Corresponding input is still RGBA sRGB, so if metadata is
// Fourth outupt may or may not be a key frame w/ metadata (up to
// encoder). Corresponding input is still I420 rec709, so if metadata is
// given, we expect same colorSpace as for the previous frame.
TEST.assert_eq(inputFrames[3].format, 'RGBA', 'inputs[3] is RGBA');
TEST.assert(isSRGB(inputFrames[3].colorSpace), 'inputs[3] is sRGB');
if ('decoderConfig' in outputMetadata[3]) {
TEST.assert(
isRec601(outputMetadata[3].decoderConfig.colorSpace),
'metadata[3] is rec601');
}

// Now decode the frames and ensure the encoder embedded the right color
// space information in the bitstream.

// VP8 doesn't have embedded color space information in the bitstream.
if (arg.codec == 'vp8')
return;

let decodedFrames = [];
const decoderInit = {
output(frame) {
decodedFrames.push(frame);
},
error(e) {
errors++;
TEST.log(e);
}
};

let decoder = new VideoDecoder(decoderInit);
for (var i = 0; i < outputChunks.length; ++i) {
if ('decoderConfig' in outputMetadata[i]) {
let config = {...outputMetadata[i].decoderConfig};

// Removes the color space provided by the encoder so that color space
// information in the underlying bitstream is exposed during decode.
config.colorSpace = {};

config.hardwareAcceleration = arg.acceleration;
decoder.configure(config);
}
decoder.decode(outputChunks[i]);
await waitForNextFrame();
}
await decoder.flush();
decoder.close();

TEST.assert_eq(
errors, 0, 'Encoding errors occurred during the decoding test');
TEST.assert_eq(
decodedFrames.length, outputChunks.length,
'Unexpected number of decoded outputs');

let colorSpace = {};
for (var i = 0; i < decodedFrames.length; ++i) {
if ('decoderConfig' in outputMetadata[i])
colorSpace = outputMetadata[i].decoderConfig.colorSpace;

TEST.assert_eq(
decodedFrames[i].colorSpace.primaries, colorSpace.primaries,
`Frame ${i} color primaries mismatch`);
TEST.assert_eq(
decodedFrames[i].colorSpace.matrix, colorSpace.matrix,
`Frame ${i} color matrix mismatch`);
if (decodedFrames[i].colorSpace.transfer != colorSpace.transfer) {
// Allow functionally equivalent matches.
TEST.assert(
colorSpace.transfer == 'smpte170m' &&
decodedFrames[i].colorSpace.transfer == 'bt709',
`Frame ${i} color transfer mismatch`)
} else {
TEST.assert_eq(
decodedFrames[i].colorSpace.transfer, colorSpace.transfer,
`Frame ${i} color transfer mismatch`);
}
TEST.assert_eq(
decodedFrames[i].colorSpace.fullRange, colorSpace.fullRange,
`Frame ${i} color fullRange mismatch`);
TEST.assert(inputFrames[3].format == 'I420', 'inputs[3] is I420');
TEST.assert(isRec709(inputFrames[3].colorSpace, 'inputs[3] is rec709'));
if('decoderConfig' in outputMetadatas[3]) {
TEST.assert(isRec709(outputMetadatas[3].decoderConfig.colorSpace), 'metadatas[3] is rec709');
}
}
</script>
Expand Down
8 changes: 1 addition & 7 deletions content/test/data/gpu/webcodecs/webcodecs_common.js
View file
Expand Up @@ -37,12 +37,6 @@ 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");
}
Expand Down Expand Up @@ -444,4 +438,4 @@ async function createFrameSource(type, width, height) {
return new ArrayBufferSource(width, height);
}
}
}
}
6 changes: 0 additions & 6 deletions content/test/gpu/gpu_tests/test_expectations/webcodecs_expectations.txt
View file
Expand Up @@ -115,12 +115,6 @@ 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 #
#######################################################################
6 changes: 0 additions & 6 deletions media/base/mac/color_space_util_mac.h
View file
Expand Up @@ -21,12 +21,6 @@ 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_

0 comments on commit 67ccd66

Please sign in to comment.