[macOS Capture] Fail gracefully if camera produces invalid MJPEG frame.
As detailed in https://crbug.com/1147867, after rebooting Logitech C920
produces a single invalid MJPEG sample buffer frame that we are unable
to transform into I420 or NV12. Subsequent frames are valid, and this
issue cannot be reproduced again until the computor is rebooted.
Prior to this CL the invalid MJPEG frame causes a DCHECK crash inside
SampleBufferTransformer. This CL makes it fail gracefully instead by
returning null if this happens.
There is no known way to cause non-MJPEG transformations to fail, so
all other code paths still DCHECK.
This fix is verified in both unit tests that use a clearly invalid
MJPEG frame, and manually using the Logitech C920 camera.
This CL unblocks making InCaptureConvertToNv12 enabled-by-default.
Bug: chromium:1147867
Change-Id: Iefb6f5407918b4a8ea81f9ce41acc211ef80bdd5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2532556
Commit-Queue: Henrik Boström <hbos@chromium.org>
Reviewed-by: Evan Shrubsole <eshr@google.com>
Reviewed-by: Ilya Nikolaevskiy <ilnik@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826728}
diff --git a/media/capture/video/mac/sample_buffer_transformer_mac.cc b/media/capture/video/mac/sample_buffer_transformer_mac.cc
index 4137f68..5e40a90 100644
--- a/media/capture/video/mac/sample_buffer_transformer_mac.cc
+++ b/media/capture/video/mac/sample_buffer_transformer_mac.cc
@@ -233,7 +233,11 @@
return nv12_planes;
}
-void ConvertFromAnyToI420(OSType source_pixel_format,
+// Returns true on success. Converting uncompressed pixel formats should never
+// fail, however MJPEG frames produces by some webcams have been observed to be
+// invalid in special circumstances (see https://crbug.com/1147867). To support
+// a graceful failure path in this case, this function may return false.
+bool ConvertFromAnyToI420(OSType source_pixel_format,
uint8_t* source_buffer_base_address,
size_t source_buffer_size,
const I420Planes& destination) {
@@ -247,7 +251,7 @@
/*crop_width*/ destination.width,
/*crop_height*/ destination.height, libyuv::kRotate0,
MacFourCCToLibyuvFourCC(source_pixel_format));
- DCHECK_EQ(result, 0);
+ return result == 0;
}
void ConvertFromI420ToNV12(const I420Planes& source,
@@ -260,10 +264,16 @@
destination.y_plane_data, destination.y_plane_stride,
destination.uv_plane_data, destination.uv_plane_stride, source.width,
source.height);
+ // A webcam has never been observed to produce invalid uncompressed pixel
+ // buffer, so we do not support a graceful failure path in this case.
DCHECK_EQ(result, 0);
}
-void ConvertFromMjpegToNV12(uint8_t* source_buffer_data_base_address,
+// Returns true on success. MJPEG frames produces by some webcams have been
+// observed to be invalid in special circumstances (see
+// https://crbug.com/1147867). To support a graceful failure path in this case,
+// this function may return false.
+bool ConvertFromMjpegToNV12(uint8_t* source_buffer_data_base_address,
size_t source_buffer_data_size,
const NV12Planes& destination) {
// Despite libyuv::MJPGToNV12() taking both source and destination sizes as
@@ -274,7 +284,7 @@
destination.y_plane_data, destination.y_plane_stride,
destination.uv_plane_data, destination.uv_plane_stride, destination.width,
destination.height, destination.width, destination.height);
- DCHECK_EQ(result, 0);
+ return result == 0;
}
void ScaleI420(const I420Planes& source, const I420Planes& destination) {
@@ -454,7 +464,10 @@
return destination_pixel_buffer;
}
// Sample buffer path - it's MJPEG. Do libyuv conversion + rescale.
- TransformSampleBuffer(sample_buffer, destination_pixel_buffer);
+ if (!TransformSampleBuffer(sample_buffer, destination_pixel_buffer)) {
+ LOG(ERROR) << "Failed to transform sample buffer.";
+ return base::ScopedCFTypeRef<CVPixelBufferRef>();
+ }
return destination_pixel_buffer;
}
@@ -548,8 +561,13 @@
i420_fullscale_buffer = EnsureI420BufferSizeAndGetPlanes(
source_width, source_height, &intermediate_i420_buffer_);
}
- ConvertFromAnyToI420(source_pixel_format, source_buffer_data_base_address,
- source_buffer_data_size, i420_fullscale_buffer);
+ if (!ConvertFromAnyToI420(source_pixel_format,
+ source_buffer_data_base_address,
+ source_buffer_data_size, i420_fullscale_buffer)) {
+ // Only MJPEG conversions are known to be able to fail. Because X is an
+ // uncompressed pixel format, this conversion should never fail.
+ NOTREACHED();
+ }
}
// Step 2: Rescale I420.
@@ -593,8 +611,11 @@
// Convert X -> I420.
i420_fullscale_buffer = EnsureI420BufferSizeAndGetPlanes(
source_width, source_height, &intermediate_i420_buffer_);
- ConvertFromAnyToI420(source_pixel_format, source_buffer_data_base_address,
- source_buffer_data_size, i420_fullscale_buffer);
+ if (!ConvertFromAnyToI420(
+ source_pixel_format, source_buffer_data_base_address,
+ source_buffer_data_size, i420_fullscale_buffer)) {
+ NOTREACHED();
+ }
}
// Convert I420 -> NV12.
if (!rescale_needed) {
@@ -615,7 +636,7 @@
}
}
-void SampleBufferTransformer::TransformSampleBuffer(
+bool SampleBufferTransformer::TransformSampleBuffer(
CMSampleBufferRef source_sample_buffer,
CVPixelBufferRef destination_pixel_buffer) {
DCHECK(transformer_ == Transformer::kLibyuv);
@@ -639,15 +660,16 @@
CVPixelBufferLockBaseAddress(destination_pixel_buffer, 0);
DCHECK_EQ(lock_status, kCVReturnSuccess);
// Convert to I420 or NV12.
+ bool success = false;
switch (destination_pixel_format_) {
case kPixelFormatI420:
- TransformSampleBufferFromMjpegToI420(
+ success = TransformSampleBufferFromMjpegToI420(
source_buffer_data_base_address, source_buffer_data_size,
source_dimensions.width, source_dimensions.height,
destination_pixel_buffer);
break;
case kPixelFormatNv12:
- TransformSampleBufferFromMjpegToNV12(
+ success = TransformSampleBufferFromMjpegToNV12(
source_buffer_data_base_address, source_buffer_data_size,
source_dimensions.width, source_dimensions.height,
destination_pixel_buffer);
@@ -658,9 +680,10 @@
// Unlock destination pixel buffer.
lock_status = CVPixelBufferUnlockBaseAddress(destination_pixel_buffer, 0);
DCHECK_EQ(lock_status, kCVReturnSuccess);
+ return success;
}
-void SampleBufferTransformer::TransformSampleBufferFromMjpegToI420(
+bool SampleBufferTransformer::TransformSampleBufferFromMjpegToI420(
uint8_t* source_buffer_data_base_address,
size_t source_buffer_data_size,
size_t source_width,
@@ -680,8 +703,10 @@
i420_fullscale_buffer = EnsureI420BufferSizeAndGetPlanes(
source_width, source_height, &intermediate_i420_buffer_);
}
- ConvertFromAnyToI420(kPixelFormatMjpeg, source_buffer_data_base_address,
- source_buffer_data_size, i420_fullscale_buffer);
+ if (!ConvertFromAnyToI420(kPixelFormatMjpeg, source_buffer_data_base_address,
+ source_buffer_data_size, i420_fullscale_buffer)) {
+ return false;
+ }
// Step 2: Rescale I420.
if (rescale_needed) {
@@ -689,9 +714,10 @@
GetI420PlanesFromPixelBuffer(destination_pixel_buffer);
ScaleI420(i420_fullscale_buffer, i420_destination_buffer);
}
+ return true;
}
-void SampleBufferTransformer::TransformSampleBufferFromMjpegToNV12(
+bool SampleBufferTransformer::TransformSampleBufferFromMjpegToNV12(
uint8_t* source_buffer_data_base_address,
size_t source_buffer_data_size,
size_t source_width,
@@ -711,8 +737,10 @@
nv12_fullscale_buffer = EnsureNV12BufferSizeAndGetPlanes(
source_width, source_height, &intermediate_nv12_buffer_);
}
- ConvertFromMjpegToNV12(source_buffer_data_base_address,
- source_buffer_data_size, nv12_fullscale_buffer);
+ if (!ConvertFromMjpegToNV12(source_buffer_data_base_address,
+ source_buffer_data_size, nv12_fullscale_buffer)) {
+ return false;
+ }
// Step 2: Rescale NV12.
if (rescale_needed) {
@@ -720,6 +748,7 @@
GetNV12PlanesFromPixelBuffer(destination_pixel_buffer);
ScaleNV12(nv12_fullscale_buffer, nv12_destination_buffer);
}
+ return true;
}
} // namespace media
diff --git a/media/capture/video/mac/sample_buffer_transformer_mac.h b/media/capture/video/mac/sample_buffer_transformer_mac.h
index 89f6659..4f9dc45 100644
--- a/media/capture/video/mac/sample_buffer_transformer_mac.h
+++ b/media/capture/video/mac/sample_buffer_transformer_mac.h
@@ -115,15 +115,15 @@
CVPixelBufferRef source_pixel_buffer,
CVPixelBufferRef destination_pixel_buffer);
// Sample buffers from the camera contain byte buffers when MJPEG is used.
- void TransformSampleBuffer(CMSampleBufferRef source_sample_buffer,
+ bool TransformSampleBuffer(CMSampleBufferRef source_sample_buffer,
CVPixelBufferRef destination_pixel_buffer);
- void TransformSampleBufferFromMjpegToI420(
+ bool TransformSampleBufferFromMjpegToI420(
uint8_t* source_buffer_data_base_address,
size_t source_buffer_data_size,
size_t source_width,
size_t source_height,
CVPixelBufferRef destination_pixel_buffer);
- void TransformSampleBufferFromMjpegToNV12(
+ bool TransformSampleBufferFromMjpegToNV12(
uint8_t* source_buffer_data_base_address,
size_t source_buffer_data_size,
size_t source_width,
diff --git a/media/capture/video/mac/sample_buffer_transformer_mac_unittest.mm b/media/capture/video/mac/sample_buffer_transformer_mac_unittest.mm
index 0ed0d7c..f6db71a3 100644
--- a/media/capture/video/mac/sample_buffer_transformer_mac_unittest.mm
+++ b/media/capture/video/mac/sample_buffer_transformer_mac_unittest.mm
@@ -81,6 +81,11 @@
constexpr uint32_t kExampleJpegScaledDownWidth = 16;
constexpr uint32_t kExampleJpegScaledDownHeight = 8;
+const uint8_t kInvalidJpegData[] = {
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+constexpr size_t kInvalidJpegDataSize = 24;
+
constexpr uint8_t kColorR = 255u;
constexpr uint8_t kColorG = 127u;
constexpr uint8_t kColorB = 63u;
@@ -172,26 +177,21 @@
return sample_buffer;
}
-base::ScopedCFTypeRef<CMSampleBufferRef> CreateExamlpeMjpegSampleBuffer() {
- // Sanity-check the example data.
- int width;
- int height;
- int result =
- libyuv::MJPGSize(kExampleJpegData, kExampleJpegDataSize, &width, &height);
- DCHECK(result == 0);
- DCHECK_EQ(width, static_cast<int>(kExampleJpegWidth));
- DCHECK_EQ(height, static_cast<int>(kExampleJpegHeight));
-
+base::ScopedCFTypeRef<CMSampleBufferRef> CreateMjpegSampleBuffer(
+ const uint8_t* mjpeg_data,
+ size_t mjpeg_data_size,
+ size_t width,
+ size_t height) {
CMBlockBufferRef data_buffer;
OSStatus status = CMBlockBufferCreateWithMemoryBlock(
- nil, const_cast<void*>(static_cast<const void*>(kExampleJpegData)),
- kExampleJpegDataSize, nil, nil, 0, kExampleJpegDataSize, 0, &data_buffer);
+ nil, const_cast<void*>(static_cast<const void*>(mjpeg_data)),
+ mjpeg_data_size, nil, nil, 0, mjpeg_data_size, 0, &data_buffer);
DCHECK(status == noErr);
CMFormatDescriptionRef format_description;
- status = CMVideoFormatDescriptionCreate(nil, kCMVideoCodecType_JPEG_OpenDML,
- kExampleJpegWidth, kExampleJpegHeight,
- nil, &format_description);
+ status =
+ CMVideoFormatDescriptionCreate(nil, kCMVideoCodecType_JPEG_OpenDML, width,
+ height, nil, &format_description);
DCHECK(status == noErr);
// Dummy information to make CMSampleBufferCreateReady() happy.
@@ -209,6 +209,24 @@
return sample_buffer;
}
+base::ScopedCFTypeRef<CMSampleBufferRef> CreateExampleMjpegSampleBuffer() {
+ // Sanity-check the example data.
+ int width;
+ int height;
+ int result =
+ libyuv::MJPGSize(kExampleJpegData, kExampleJpegDataSize, &width, &height);
+ DCHECK(result == 0);
+ DCHECK_EQ(width, static_cast<int>(kExampleJpegWidth));
+ DCHECK_EQ(height, static_cast<int>(kExampleJpegHeight));
+ return CreateMjpegSampleBuffer(kExampleJpegData, kExampleJpegDataSize,
+ kExampleJpegWidth, kExampleJpegHeight);
+}
+
+base::ScopedCFTypeRef<CMSampleBufferRef> CreateInvalidMjpegSampleBuffer() {
+ return CreateMjpegSampleBuffer(kInvalidJpegData, kInvalidJpegDataSize,
+ kExampleJpegWidth, kExampleJpegHeight);
+}
+
} // namespace
class SampleBufferTransformerPixelTransferTest
@@ -332,7 +350,7 @@
OSType output_pixel_format = GetParam();
base::ScopedCFTypeRef<CMSampleBufferRef> input_sample_buffer =
- CreateExamlpeMjpegSampleBuffer();
+ CreateExampleMjpegSampleBuffer();
std::unique_ptr<SampleBufferTransformer> transformer =
SampleBufferTransformer::Create();
transformer->Reconfigure(SampleBufferTransformer::Transformer::kLibyuv,
@@ -351,7 +369,7 @@
OSType output_pixel_format = GetParam();
base::ScopedCFTypeRef<CMSampleBufferRef> input_sample_buffer =
- CreateExamlpeMjpegSampleBuffer();
+ CreateExampleMjpegSampleBuffer();
std::unique_ptr<SampleBufferTransformer> transformer =
SampleBufferTransformer::Create();
transformer->Reconfigure(SampleBufferTransformer::Transformer::kLibyuv,
@@ -368,6 +386,22 @@
PixelBufferIsSingleColor(output_pixel_buffer, kColorR, kColorG, kColorB));
}
+TEST_P(SampleBufferTransformerMjpegTest,
+ AttemptingToTransformInvalidMjpegFailsGracefully) {
+ OSType output_pixel_format = GetParam();
+
+ base::ScopedCFTypeRef<CMSampleBufferRef> input_sample_buffer =
+ CreateInvalidMjpegSampleBuffer();
+ std::unique_ptr<SampleBufferTransformer> transformer =
+ SampleBufferTransformer::Create();
+ transformer->Reconfigure(SampleBufferTransformer::Transformer::kLibyuv,
+ output_pixel_format, kExampleJpegWidth,
+ kExampleJpegHeight, 1);
+ base::ScopedCFTypeRef<CVPixelBufferRef> output_pixel_buffer =
+ transformer->Transform(input_sample_buffer);
+ EXPECT_FALSE(output_pixel_buffer);
+}
+
INSTANTIATE_TEST_SUITE_P(SampleBufferTransformerTest,
SampleBufferTransformerMjpegTest,
SupportedOutputFormats(),
@@ -444,7 +478,7 @@
IOSurfaceGetPixelFormat(CVPixelBufferGetIOSurface(output_buffer)));
output_buffer = transformer->AutoReconfigureAndTransform(
- CreateExamlpeMjpegSampleBuffer());
+ CreateExampleMjpegSampleBuffer());
EXPECT_EQ(kPixelFormatNv12, transformer->destination_pixel_format());
EXPECT_EQ(kPixelFormatNv12,
IOSurfaceGetPixelFormat(CVPixelBufferGetIOSurface(output_buffer)));
@@ -483,7 +517,7 @@
transformer->transformer());
output_buffer = transformer->AutoReconfigureAndTransform(
- CreateExamlpeMjpegSampleBuffer());
+ CreateExampleMjpegSampleBuffer());
EXPECT_EQ(SampleBufferTransformer::Transformer::kLibyuv,
transformer->transformer());
}
