Skip to content

Commit

Permalink
Remove uses of implicit conversion of ScopedTypeRef in /media
Browse files Browse the repository at this point in the history 
Implicit unwrapping of a scoper to its underlying pointer is dangerous
and that capability is being removed. This converts uses of implicit
conversion to be explicit in preparation for its removal, and performs
other cleanup and modernization.

Bug: 1495439
Low-Coverage-Reason: LARGE_SCALE_REFACTOR Switching from implicit to explicit
Change-Id: I9d20564c781493f05f7e91b0bef9f0037450d4bb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4981008
Commit-Queue: Mark Foltz <mfoltz@chromium.org>
Reviewed-by: Mark Foltz <mfoltz@chromium.org>
Reviewed-by: Henrik Boström <hbos@chromium.org>
Auto-Submit: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1216265}
  • Loading branch information
Avi Drissman authored and Chromium LUCI CQ committed Oct 27, 2023
1 parent 590d7f8 commit 819f48e
Show file tree
Hide file tree
Showing 38 changed files with 605 additions and 514 deletions.
9 changes: 5 additions & 4 deletions media/audio/mac/audio_loopback_input_mac_impl.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ - (void)stream:(SCStream*)stream
base::apple::ScopedCFTypeRef<CMSampleBufferRef> sample_buffer,
const double volume) {
const CMBlockBufferRef block_buffer =
CMSampleBufferGetDataBuffer(sample_buffer);
CMSampleBufferGetDataBuffer(sample_buffer.get());
if (!block_buffer) {
VLOG(1) << "Sample buffer is empty.";
return;
Expand All @@ -491,9 +491,9 @@ - (void)stream:(SCStream*)stream
}

const CMTime time_stamp =
CMSampleBufferGetPresentationTimeStamp(sample_buffer);
CMSampleBufferGetPresentationTimeStamp(sample_buffer.get());
const CMFormatDescriptionRef format_description =
CMSampleBufferGetFormatDescription(sample_buffer);
CMSampleBufferGetFormatDescription(sample_buffer.get());
const AudioStreamBasicDescription* audio_description =
CMAudioFormatDescriptionGetStreamBasicDescription(format_description);

Expand All @@ -503,7 +503,8 @@ - (void)stream:(SCStream*)stream
CHECK_EQ(audio_description->mBytesPerFrame, sizeof(float))
<< "Expected non-interleaved data.";

const size_t total_frame_count = CMSampleBufferGetNumSamples(sample_buffer);
const size_t total_frame_count =
CMSampleBufferGetNumSamples(sample_buffer.get());

base::TimeTicks capture_time;
capture_time += base::Seconds(CMTimeGetSeconds(time_stamp));
Expand Down
5 changes: 3 additions & 2 deletions media/audio/mac/audio_loopback_input_mac_unittest.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,8 @@ void StartSCStreamMocking(SCStream* stream,

base::apple::ScopedCFTypeRef<CMSampleBufferRef> sample_buffer;
CMAudioSampleBufferCreateReadyWithPacketDescriptions(
kCFAllocatorDefault, block_buffer, format_description, kFramesPerBuffer,
kCFAllocatorDefault, block_buffer.get(), format_description.get(),
kFramesPerBuffer,
CMTimeMakeWithSeconds(
base::TimeTicks::Now().since_origin().InMicrosecondsF(), 1000000),
NULL, sample_buffer.InitializeInto());
Expand All @@ -206,7 +207,7 @@ void SendAudioSample(std::array<float, 2 * kFramesPerBuffer> buffer) {
// |stream| is not needed.
SCStream* stream = nil;
[stream_output stream:stream
didOutputSampleBuffer:CreateStereoAudioSampleBuffer(buffer)
didOutputSampleBuffer:CreateStereoAudioSampleBuffer(buffer).get()
ofType:SCStreamOutputTypeAudio];
}
}
Expand Down
8 changes: 4 additions & 4 deletions media/audio/mac/audio_low_latency_input_mac.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,11 +142,11 @@ static AudioDeviceID FindFirstOutputSubdevice(
}

AudioDeviceID output_subdevice_id = kAudioObjectUnknown;
DCHECK_EQ(CFGetTypeID(subdevices), CFArrayGetTypeID());
const CFIndex count = CFArrayGetCount(subdevices);
DCHECK_EQ(CFGetTypeID(subdevices.get()), CFArrayGetTypeID());
const CFIndex count = CFArrayGetCount(subdevices.get());
for (CFIndex i = 0; i != count; ++i) {
CFStringRef value =
base::apple::CFCast<CFStringRef>(CFArrayGetValueAtIndex(subdevices, i));
CFStringRef value = base::apple::CFCast<CFStringRef>(
CFArrayGetValueAtIndex(subdevices.get(), i));
if (value) {
std::string uid = base::SysCFStringRefToUTF8(value);
output_subdevice_id = AudioManagerMac::GetAudioDeviceIdByUId(false, uid);
Expand Down
14 changes: 8 additions & 6 deletions media/base/mac/video_frame_mac.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,12 @@ void SetCvPixelBufferColorSpace(const gfx::ColorSpace& frame_cs,
if (frame->CvPixelBuffer()) {
pixel_buffer.reset(frame->CvPixelBuffer(), base::scoped_policy::RETAIN);
if (!IsAcceptableCvPixelFormat(
frame->format(), CVPixelBufferGetPixelFormatType(pixel_buffer))) {
frame->format(),
CVPixelBufferGetPixelFormatType(pixel_buffer.get()))) {
DLOG(ERROR) << "Dropping CVPixelBuffer w/ incorrect format.";
pixel_buffer.reset();
} else {
SetCvPixelBufferColorSpace(frame->ColorSpace(), pixel_buffer);
SetCvPixelBufferColorSpace(frame->ColorSpace(), pixel_buffer.get());
}
return pixel_buffer;
}
Expand All @@ -129,19 +130,20 @@ void SetCvPixelBufferColorSpace(const gfx::ColorSpace& frame_cs,
gfx::ScopedIOSurface io_surface = handle.io_surface;
if (io_surface) {
CVReturn cv_return = CVPixelBufferCreateWithIOSurface(
nullptr, io_surface, nullptr, pixel_buffer.InitializeInto());
nullptr, io_surface.get(), nullptr,
pixel_buffer.InitializeInto());
if (cv_return != kCVReturnSuccess) {
DLOG(ERROR) << "CVPixelBufferCreateWithIOSurface failed: "
<< cv_return;
pixel_buffer.reset();
}
if (!IsAcceptableCvPixelFormat(
frame->format(),
CVPixelBufferGetPixelFormatType(pixel_buffer))) {
CVPixelBufferGetPixelFormatType(pixel_buffer.get()))) {
DLOG(ERROR) << "Dropping CVPixelBuffer w/ incorrect format.";
pixel_buffer.reset();
} else {
SetCvPixelBufferColorSpace(frame->ColorSpace(), pixel_buffer);
SetCvPixelBufferColorSpace(frame->ColorSpace(), pixel_buffer.get());
}
return pixel_buffer;
}
Expand Down Expand Up @@ -225,7 +227,7 @@ void SetCvPixelBufferColorSpace(const gfx::ColorSpace& frame_cs,
// reference count manually. The release callback set on the pixel buffer will
// release the frame.
frame->AddRef();
SetCvPixelBufferColorSpace(frame->ColorSpace(), pixel_buffer);
SetCvPixelBufferColorSpace(frame->ColorSpace(), pixel_buffer.get());
return pixel_buffer;
}

Expand Down
45 changes: 25 additions & 20 deletions media/base/mac/video_frame_mac_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,20 +46,23 @@ TEST(VideoFrameMac, CheckBasicAttributes) {
const gfx::Size coded_size = frame->coded_size();
const VideoPixelFormat format = frame->format();

EXPECT_EQ(coded_size.width(), static_cast<int>(CVPixelBufferGetWidth(pb)));
EXPECT_EQ(coded_size.height(), static_cast<int>(CVPixelBufferGetHeight(pb)));
EXPECT_EQ(VideoFrame::NumPlanes(format), CVPixelBufferGetPlaneCount(pb));

CVPixelBufferLockBaseAddress(pb, 0);
EXPECT_EQ(coded_size.width(),
static_cast<int>(CVPixelBufferGetWidth(pb.get())));
EXPECT_EQ(coded_size.height(),
static_cast<int>(CVPixelBufferGetHeight(pb.get())));
EXPECT_EQ(VideoFrame::NumPlanes(format),
CVPixelBufferGetPlaneCount(pb.get()));

CVPixelBufferLockBaseAddress(pb.get(), 0);
for (size_t i = 0; i < VideoFrame::NumPlanes(format); ++i) {
const gfx::Size plane_size = VideoFrame::PlaneSize(format, i, coded_size);
EXPECT_EQ(plane_size.width(),
static_cast<int>(CVPixelBufferGetWidthOfPlane(pb, i)));
static_cast<int>(CVPixelBufferGetWidthOfPlane(pb.get(), i)));
EXPECT_EQ(plane_size.height(),
static_cast<int>(CVPixelBufferGetHeightOfPlane(pb, i)));
EXPECT_EQ(frame->data(i), CVPixelBufferGetBaseAddressOfPlane(pb, i));
static_cast<int>(CVPixelBufferGetHeightOfPlane(pb.get(), i)));
EXPECT_EQ(frame->data(i), CVPixelBufferGetBaseAddressOfPlane(pb.get(), i));
}
CVPixelBufferUnlockBaseAddress(pb, 0);
CVPixelBufferUnlockBaseAddress(pb.get(), 0);
}

TEST(VideoFrameMac, CheckFormats) {
Expand All @@ -79,7 +82,8 @@ TEST(VideoFrameMac, CheckFormats) {
ASSERT_TRUE(frame.get());
auto pb = WrapVideoFrameInCVPixelBuffer(frame);
if (format_pair.corevideo) {
EXPECT_EQ(format_pair.corevideo, CVPixelBufferGetPixelFormatType(pb));
EXPECT_EQ(format_pair.corevideo,
CVPixelBufferGetPixelFormatType(pb.get()));
} else {
EXPECT_EQ(nullptr, pb.get());
}
Expand Down Expand Up @@ -159,25 +163,26 @@ TEST(VideoFrameMac, CorrectlyWrapsFramesWithPadding) {
auto pb = WrapVideoFrameInCVPixelBuffer(frame);
ASSERT_TRUE(pb.get());
EXPECT_EQ(kCVPixelFormatType_420YpCbCr8Planar,
CVPixelBufferGetPixelFormatType(pb));
EXPECT_EQ(visible_rect.width(), static_cast<int>(CVPixelBufferGetWidth(pb)));
CVPixelBufferGetPixelFormatType(pb.get()));
EXPECT_EQ(visible_rect.width(),
static_cast<int>(CVPixelBufferGetWidth(pb.get())));
EXPECT_EQ(visible_rect.height(),
static_cast<int>(CVPixelBufferGetHeight(pb)));
static_cast<int>(CVPixelBufferGetHeight(pb.get())));

CVPixelBufferLockBaseAddress(pb, 0);
CVPixelBufferLockBaseAddress(pb.get(), 0);
for (size_t i = 0; i < VideoFrame::NumPlanes(frame->format()); ++i) {
const gfx::Size plane_size =
VideoFrame::PlaneSize(frame->format(), i, visible_rect.size());
EXPECT_EQ(plane_size.width(),
static_cast<int>(CVPixelBufferGetWidthOfPlane(pb, i)));
static_cast<int>(CVPixelBufferGetWidthOfPlane(pb.get(), i)));
EXPECT_EQ(plane_size.height(),
static_cast<int>(CVPixelBufferGetHeightOfPlane(pb, i)));
static_cast<int>(CVPixelBufferGetHeightOfPlane(pb.get(), i)));

uint8_t* plane_ptr =
reinterpret_cast<uint8_t*>(CVPixelBufferGetBaseAddressOfPlane(pb, i));
uint8_t* plane_ptr = reinterpret_cast<uint8_t*>(
CVPixelBufferGetBaseAddressOfPlane(pb.get(), i));
EXPECT_EQ(frame->visible_data(i), plane_ptr);
const int stride =
static_cast<int>(CVPixelBufferGetBytesPerRowOfPlane(pb, i));
static_cast<int>(CVPixelBufferGetBytesPerRowOfPlane(pb.get(), i));
EXPECT_EQ(frame->stride(i), stride);
const int offset = kVisibleRectOffset / ((i == 0) ? 1 : 2);
for (int h = 0; h < plane_size.height(); ++h) {
Expand All @@ -189,7 +194,7 @@ TEST(VideoFrameMac, CorrectlyWrapsFramesWithPadding) {
}
}
}
CVPixelBufferUnlockBaseAddress(pb, 0);
CVPixelBufferUnlockBaseAddress(pb.get(), 0);
}

} // namespace media
20 changes: 11 additions & 9 deletions media/base/mac/videotoolbox_helpers.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,8 @@ bool CopySampleBufferToAnnexBBuffer(VideoCodec codec,
// Copy all the NAL units. In the process convert them from AVCC/HVCC format
// (length header) to AnnexB format (start code).
char* bb_data;
status =
CMBlockBufferGetDataPointer(contiguous_bb, 0, nullptr, nullptr, &bb_data);
status = CMBlockBufferGetDataPointer(contiguous_bb.get(), 0, nullptr, nullptr,
&bb_data);
if (status != noErr) {
DLOG(ERROR) << " CMBlockBufferGetDataPointer failed: " << status;
return false;
Expand Down Expand Up @@ -307,40 +307,42 @@ bool SessionPropertySetter::IsSupported(CFStringRef key) {
DCHECK(session_);
if (!supported_keys_) {
CFDictionaryRef dict_ref;
if (VTSessionCopySupportedPropertyDictionary(session_, &dict_ref) == noErr)
if (VTSessionCopySupportedPropertyDictionary(session_.get(), &dict_ref) ==
noErr) {
supported_keys_.reset(dict_ref);
}
}
return supported_keys_ && CFDictionaryContainsKey(supported_keys_, key);
return supported_keys_ && CFDictionaryContainsKey(supported_keys_.get(), key);
}

bool SessionPropertySetter::Set(CFStringRef key, int32_t value) {
DCHECK(session_);
base::apple::ScopedCFTypeRef<CFNumberRef> cfvalue(
CFNumberCreate(nullptr, kCFNumberSInt32Type, &value));
return VTSessionSetProperty(session_, key, cfvalue) == noErr;
return VTSessionSetProperty(session_.get(), key, cfvalue.get()) == noErr;
}

bool SessionPropertySetter::Set(CFStringRef key, bool value) {
DCHECK(session_);
CFBooleanRef cfvalue = (value) ? kCFBooleanTrue : kCFBooleanFalse;
return VTSessionSetProperty(session_, key, cfvalue) == noErr;
return VTSessionSetProperty(session_.get(), key, cfvalue) == noErr;
}

bool SessionPropertySetter::Set(CFStringRef key, double value) {
DCHECK(session_);
base::apple::ScopedCFTypeRef<CFNumberRef> cfvalue(
CFNumberCreate(nullptr, kCFNumberDoubleType, &value));
return VTSessionSetProperty(session_, key, cfvalue) == noErr;
return VTSessionSetProperty(session_.get(), key, cfvalue.get()) == noErr;
}

bool SessionPropertySetter::Set(CFStringRef key, CFStringRef value) {
DCHECK(session_);
return VTSessionSetProperty(session_, key, value) == noErr;
return VTSessionSetProperty(session_.get(), key, value) == noErr;
}

bool SessionPropertySetter::Set(CFStringRef key, CFArrayRef value) {
DCHECK(session_);
return VTSessionSetProperty(session_, key, value) == noErr;
return VTSessionSetProperty(session_.get(), key, value) == noErr;
}

} // namespace video_toolbox
Expand Down
16 changes: 8 additions & 8 deletions media/base/video_frame.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -741,20 +741,20 @@ scoped_refptr<VideoFrame> VideoFrame::WrapUnacceleratedIOSurface(
}

// Only support NV12 IOSurfaces.
const OSType cv_pixel_format = IOSurfaceGetPixelFormat(io_surface);
const OSType cv_pixel_format = IOSurfaceGetPixelFormat(io_surface.get());
if (cv_pixel_format != kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange) {
DLOG(ERROR) << "Invalid (non-NV12) pixel format.";
return nullptr;
}
const VideoPixelFormat pixel_format = PIXEL_FORMAT_NV12;

// Retrieve the layout parameters for |io_surface_|.
const size_t num_planes = IOSurfaceGetPlaneCount(io_surface);
const gfx::Size size(IOSurfaceGetWidth(io_surface),
IOSurfaceGetHeight(io_surface));
const size_t num_planes = IOSurfaceGetPlaneCount(io_surface.get());
const gfx::Size size(IOSurfaceGetWidth(io_surface.get()),
IOSurfaceGetHeight(io_surface.get()));
std::vector<int32_t> strides;
for (size_t i = 0; i < num_planes; ++i)
strides.push_back(IOSurfaceGetBytesPerRowOfPlane(io_surface, i));
strides.push_back(IOSurfaceGetBytesPerRowOfPlane(io_surface.get(), i));
absl::optional<VideoFrameLayout> layout =
media::VideoFrameLayout::CreateWithStrides(pixel_format, size, strides);
if (!layout) {
Expand All @@ -771,21 +771,21 @@ scoped_refptr<VideoFrame> VideoFrame::WrapUnacceleratedIOSurface(
// Lock the IOSurface for CPU read access. After the VideoFrame is created,
// add a destruction callback to unlock the IOSurface.
kern_return_t lock_result =
IOSurfaceLock(io_surface, kIOSurfaceLockReadOnly, nullptr);
IOSurfaceLock(io_surface.get(), kIOSurfaceLockReadOnly, nullptr);
if (lock_result != kIOReturnSuccess) {
DLOG(ERROR) << "Failed to lock IOSurface.";
return nullptr;
}
auto unlock_lambda =
[](base::apple::ScopedCFTypeRef<IOSurfaceRef> io_surface) {
IOSurfaceUnlock(io_surface, kIOSurfaceLockReadOnly, nullptr);
IOSurfaceUnlock(io_surface.get(), kIOSurfaceLockReadOnly, nullptr);
};

scoped_refptr<VideoFrame> frame =
new VideoFrame(*layout, storage_type, visible_rect, size, timestamp);
for (size_t i = 0; i < num_planes; ++i) {
frame->data_[i] = reinterpret_cast<uint8_t*>(
IOSurfaceGetBaseAddressOfPlane(io_surface, i));
IOSurfaceGetBaseAddressOfPlane(io_surface.get(), i));
}
frame->AddDestructionObserver(
base::BindOnce(unlock_lambda, std::move(io_surface)));
Expand Down
13 changes: 7 additions & 6 deletions media/capture/video/apple/gpu_memory_buffer_tracker_apple.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ bool GpuMemoryBufferTrackerApple::Init(const gfx::Size& dimensions,
if ((io_surface_ =
CreateIOSurface(dimensions, gfx::BufferFormat::YUV_420_BIPLANAR,
/*should_clear=*/false))) {
DVLOG(2) << __func__ << " id " << IOSurfaceGetID(io_surface_);
DVLOG(2) << __func__ << " id " << IOSurfaceGetID(io_surface_.get());
return true;
} else {
LOG(ERROR) << "Unable to create IOSurface!";
Expand All @@ -41,7 +41,8 @@ bool GpuMemoryBufferTrackerApple::IsSameGpuMemoryBuffer(
if (!is_external_io_surface_) {
return false;
}
return IOSurfaceGetID(io_surface_) == IOSurfaceGetID(handle.io_surface);
return IOSurfaceGetID(io_surface_.get()) ==
IOSurfaceGetID(handle.io_surface.get());
}

bool GpuMemoryBufferTrackerApple::IsReusableForFormat(
Expand All @@ -51,13 +52,13 @@ bool GpuMemoryBufferTrackerApple::IsReusableForFormat(
if (is_external_io_surface_) {
return false;
}
gfx::Size surface_size(IOSurfaceGetWidth(io_surface_),
IOSurfaceGetHeight(io_surface_));
gfx::Size surface_size(IOSurfaceGetWidth(io_surface_.get()),
IOSurfaceGetHeight(io_surface_.get()));
return format == PIXEL_FORMAT_NV12 && dimensions == surface_size;
}

uint32_t GpuMemoryBufferTrackerApple::GetMemorySizeInBytes() {
return IOSurfaceGetAllocSize(io_surface_);
return IOSurfaceGetAllocSize(io_surface_.get());
}

std::unique_ptr<VideoCaptureBufferHandle>
Expand All @@ -80,7 +81,7 @@ GpuMemoryBufferTrackerApple::DuplicateAsMojoBuffer() {

gfx::GpuMemoryBufferHandle
GpuMemoryBufferTrackerApple::GetGpuMemoryBufferHandle() {
DVLOG(2) << __func__ << " id " << IOSurfaceGetID(io_surface_);
DVLOG(2) << __func__ << " id " << IOSurfaceGetID(io_surface_.get());
gfx::GpuMemoryBufferHandle gmb_handle;
gmb_handle.type = gfx::GpuMemoryBufferType::IO_SURFACE_BUFFER;
gmb_handle.id = gfx::GpuMemoryBufferHandle::kInvalidId;
Expand Down
7 changes: 4 additions & 3 deletions media/capture/video/apple/pixel_buffer_pool.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ base::apple::ScopedCFTypeRef<CVPixelBufferRef> PixelBufferPool::CreateBuffer() {
CVReturn buffer_creation_error;
if (!max_buffers_.has_value()) {
buffer_creation_error = CVPixelBufferPoolCreatePixelBuffer(
nil, buffer_pool_, buffer.InitializeInto());
nil, buffer_pool_.get(), buffer.InitializeInto());
} else {
// Specify the allocation threshold using auxiliary attributes.
CFStringRef attribute_keys[] = {kCVPixelBufferPoolAllocationThresholdKey};
Expand All @@ -126,7 +126,7 @@ base::apple::ScopedCFTypeRef<CVPixelBufferRef> PixelBufferPool::CreateBuffer() {
attribute_count, &kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
buffer_creation_error = CVPixelBufferPoolCreatePixelBufferWithAuxAttributes(
nil, buffer_pool_, attributes.get(), buffer.InitializeInto());
nil, buffer_pool_.get(), attributes.get(), buffer.InitializeInto());
}
if (buffer_creation_error == kCVReturnWouldExceedAllocationThreshold) {
LOG(ERROR) << "Cannot exceed the pool's maximum buffer count";
Expand Down Expand Up @@ -154,7 +154,8 @@ base::apple::ScopedCFTypeRef<CVPixelBufferRef> PixelBufferPool::CreateBuffer() {

void PixelBufferPool::Flush() {
DCHECK(buffer_pool_);
CVPixelBufferPoolFlush(buffer_pool_, kCVPixelBufferPoolFlushExcessBuffers);
CVPixelBufferPoolFlush(buffer_pool_.get(),
kCVPixelBufferPoolFlushExcessBuffers);
}

} // namespace media

0 comments on commit 819f48e

Please sign in to comment.