[FFmpeg-devel] [PATCH v8 2/3] libavdevice/avfoundation.m: Replace mutex-based concurrency handling in avfoundation.m by a thread-safe fifo queue with maximum length
Marvin Scholz
epirat07 at gmail.com
Wed Jan 5 16:50:31 EET 2022
On 31 Dec 2021, at 18:43, Romain Beauxis wrote:
> * Use a CMSimpleQueueEnqueue with maximum length to queue and process
> incoming audio and video frames.
> * Log avfoundation errors.
> * Use AVERROR_EXTERNAL instead of AVERROR(EIO) in avfoundation errors.
>
> Signed-off-by: Romain Beauxis <toots at rastageeks.org>
> —
> [Sorry for the noise but an issue came up with the previous set]
>
> This is the second patch of a series of 3 that fix, cleanup and
> enhance the
> avfoundation implementation for libavdevice.
>
> These patches come from an actual user-facing application relying on
> libavdevice’s implementation of avfoundation audio input. Without
> them,
> Avfoundation is practically unusable as it will:
> * Refuse to process certain specific audio input format that are
> actually
> returned by the OS for some users (packed PCM audio)
> * Drop audio frames, resulting in corrupted audio input. This might
> have been
> unnoticed with video frames but this makes avfoundation essentially
> unusable
> for audio.
>
> The patches are now being included in our production build so they are
> tested
> and usable in production.
>
> Changelog for this patch:
> * v2: None
> * v3: None
> * v4: None
> * v5: Fix indentation/wrapping
> * v6: None
> * v7: Removed use of kAudioConverterPropertyCalculateOutputBufferSize
> to calculate output buffer size. The calculation is trivial and this
> call was
> randomly failing for no reason
> * v8: Fix memory leak when video or audio queue is full
>
> libavdevice/avfoundation.m | 194 +++++++++++++++++++------------------
> 1 file changed, 100 insertions(+), 94 deletions(-)
>
> diff --git a/libavdevice/avfoundation.m b/libavdevice/avfoundation.m
> index 738cd93375..36f9fdc53d 100644
> --- a/libavdevice/avfoundation.m
> +++ b/libavdevice/avfoundation.m
> @@ -26,7 +26,7 @@
> */
>
> #import <AVFoundation/AVFoundation.h>
> -#include <pthread.h>
> +#import <CoreMedia/CoreMedia.h>
>
> #include "libavutil/channel_layout.h"
> #include "libavutil/pixdesc.h"
> @@ -39,6 +39,11 @@
> #include "libavutil/imgutils.h"
> #include "avdevice.h"
>
> +#define av_log_avfoundation_error(s, str, err) \
> + av_log(s, AV_LOG_ERROR, "Avfoundation: %s, %s\n", str, \
nitpick: should probably be AVFoundation, no?
> + [[[NSError errorWithDomain:NSOSStatusErrorDomain code:err
> userInfo:nil] localizedDescription] UTF8String] \
> + )
> +
The errorWithDomain: returns an autorelease NSError, however there is no
autorelease pool.
Either make this a function with an @autorelease pool or use [[…
alloc] init…] instead, and
release the NSError.
> static const int avf_time_base = 1000000;
>
> static const AVRational avf_time_base_q = {
> @@ -80,13 +85,12 @@
> { AV_PIX_FMT_NONE, 0 }
> };
>
> +#define MAX_QUEUED_FRAMES 10
> +
> typedef struct
> {
> AVClass* class;
>
> - int frames_captured;
> - int audio_frames_captured;
> - pthread_mutex_t frame_lock;
> id avf_delegate;
> id avf_audio_delegate;
>
> @@ -122,8 +126,8 @@
> AVCaptureSession *capture_session;
> AVCaptureVideoDataOutput *video_output;
> AVCaptureAudioDataOutput *audio_output;
> - CMSampleBufferRef current_frame;
> - CMSampleBufferRef current_audio_frame;
> + CMSimpleQueueRef audio_frames_queue;
> + CMSimpleQueueRef video_frames_queue;
>
> AVCaptureDevice *observed_device;
> #if !TARGET_OS_IPHONE && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070
> @@ -132,16 +136,6 @@
> int observed_quit;
> } AVFContext;
>
> -static void lock_frames(AVFContext* ctx)
> -{
> - pthread_mutex_lock(&ctx->frame_lock);
> -}
> -
> -static void unlock_frames(AVFContext* ctx)
> -{
> - pthread_mutex_unlock(&ctx->frame_lock);
> -}
> -
> /** FrameReciever class - delegate for AVCaptureSession
> */
> @interface AVFFrameReceiver : NSObject
> @@ -219,17 +213,13 @@ - (void) captureOutput:(AVCaptureOutput
> *)captureOutput
> didOutputSampleBuffer:(CMSampleBufferRef)videoFrame
> fromConnection:(AVCaptureConnection *)connection
> {
> - lock_frames(_context);
> + OSStatus ret = CMSimpleQueueEnqueue(_context->video_frames_queue,
> videoFrame);
>
> - if (_context->current_frame != nil) {
> - CFRelease(_context->current_frame);
> + if (ret != noErr) {
> + av_log_avfoundation_error(_context, "Error while queueing video
> frame", ret);
> }
>
> - _context->current_frame =
> (CMSampleBufferRef)CFRetain(videoFrame);
> -
> - unlock_frames(_context);
> -
> - ++_context->frames_captured;
> + CFRetain(videoFrame);
> }
>
> @end
> @@ -263,17 +253,13 @@ - (void) captureOutput:(AVCaptureOutput
> *)captureOutput
> didOutputSampleBuffer:(CMSampleBufferRef)audioFrame
> fromConnection:(AVCaptureConnection *)connection
> {
> - lock_frames(_context);
> + OSStatus ret = CMSimpleQueueEnqueue(_context->audio_frames_queue,
> audioFrame);
>
> - if (_context->current_audio_frame != nil) {
> - CFRelease(_context->current_audio_frame);
> + if (ret != noErr) {
> + av_log_avfoundation_error(_context, "Error while queueing audio
> frame", ret);
> }
>
> - _context->current_audio_frame =
> (CMSampleBufferRef)CFRetain(audioFrame);
> -
> - unlock_frames(_context);
> -
> - ++_context->audio_frames_captured;
> + CFRetain(audioFrame);
> }
>
> @end
> @@ -288,6 +274,30 @@ static void destroy_context(AVFContext* ctx)
> [ctx->avf_delegate release];
> [ctx->avf_audio_delegate release];
>
> + CMSampleBufferRef frame;
> +
> + if (ctx->video_frames_queue) {
> + frame =
> (CMSampleBufferRef)CMSimpleQueueDequeue(ctx->video_frames_queue);
> + while (frame) {
> + CFRelease(frame);
> + frame =
> (CMSampleBufferRef)CMSimpleQueueDequeue(ctx->video_frames_queue);
> + }
> +
> + CFRelease(ctx->video_frames_queue);
> + ctx->video_frames_queue = NULL;
> + }
> +
> + if (ctx->audio_frames_queue) {
> + frame =
> (CMSampleBufferRef)CMSimpleQueueDequeue(ctx->audio_frames_queue);
> + while (frame) {
> + CFRelease(frame);
> + frame =
> (CMSampleBufferRef)CMSimpleQueueDequeue(ctx->audio_frames_queue);
> + }
> +
> + CFRelease(ctx->audio_frames_queue);
> + ctx->audio_frames_queue = NULL;
> + }
> +
> ctx->capture_session = NULL;
> ctx->video_output = NULL;
> ctx->audio_output = NULL;
> @@ -298,12 +308,6 @@ static void destroy_context(AVFContext* ctx)
> AudioConverterDispose(ctx->audio_converter);
> ctx->audio_converter = NULL;
> }
> -
> - pthread_mutex_destroy(&ctx->frame_lock);
> -
> - if (ctx->current_frame) {
> - CFRelease(ctx->current_frame);
> - }
> }
>
> static void parse_device_name(AVFormatContext *s)
> @@ -631,18 +635,18 @@ static int get_video_config(AVFormatContext *s)
> }
>
> // Take stream info from the first frame.
> - while (ctx->frames_captured < 1) {
> + while (CMSimpleQueueGetCount(ctx->video_frames_queue) < 1) {
> CFRunLoopRunInMode(kCFRunLoopDefaultMode, 0.1, YES);
> }
>
> - lock_frames(ctx);
> + CMSampleBufferRef frame =
> (CMSampleBufferRef)CMSimpleQueueDequeue(ctx->video_frames_queue);
>
> ctx->video_stream_index = stream->index;
>
> avpriv_set_pts_info(stream, 64, 1, avf_time_base);
>
> - image_buffer = CMSampleBufferGetImageBuffer(ctx->current_frame);
> - block_buffer = CMSampleBufferGetDataBuffer(ctx->current_frame);
> + image_buffer = CMSampleBufferGetImageBuffer(frame);
> + block_buffer = CMSampleBufferGetDataBuffer(frame);
>
> if (image_buffer) {
> image_buffer_size = CVImageBufferGetEncodedSize(image_buffer);
> @@ -658,10 +662,7 @@ static int get_video_config(AVFormatContext *s)
> stream->codecpar->format = ctx->pixel_format;
> }
>
> - CFRelease(ctx->current_frame);
> - ctx->current_frame = nil;
> -
> - unlock_frames(ctx);
> + CFRelease(frame);
>
> return 0;
> }
> @@ -681,27 +682,27 @@ static int get_audio_config(AVFormatContext *s)
> }
>
> // Take stream info from the first frame.
> - while (ctx->audio_frames_captured < 1) {
> + while (CMSimpleQueueGetCount(ctx->audio_frames_queue) < 1) {
> CFRunLoopRunInMode(kCFRunLoopDefaultMode, 0.1, YES);
> }
>
> - lock_frames(ctx);
> + CMSampleBufferRef frame =
> (CMSampleBufferRef)CMSimpleQueueDequeue(ctx->audio_frames_queue);
>
> ctx->audio_stream_index = stream->index;
>
> avpriv_set_pts_info(stream, 64, 1, avf_time_base);
>
> - format_desc =
> CMSampleBufferGetFormatDescription(ctx->current_audio_frame);
> + format_desc = CMSampleBufferGetFormatDescription(frame);
> const AudioStreamBasicDescription *input_format =
> CMAudioFormatDescriptionGetStreamBasicDescription(format_desc);
>
> if (!input_format) {
> - unlock_frames(ctx);
> + CFRelease(frame);
> av_log(s, AV_LOG_ERROR, "audio format not available\n");
> return 1;
> }
>
> if (input_format->mFormatID != kAudioFormatLinearPCM) {
> - unlock_frames(ctx);
> + CFRelease(frame);
> av_log(s, AV_LOG_ERROR, "only PCM audio format are supported
> at the moment\n");
> return 1;
> }
> @@ -781,16 +782,13 @@ static int get_audio_config(AVFormatContext *s)
> if (must_convert) {
> OSStatus ret = AudioConverterNew(input_format, &output_format,
> &ctx->audio_converter);
> if (ret != noErr) {
> - unlock_frames(ctx);
> - av_log(s, AV_LOG_ERROR, "Error while allocating audio
> converter\n");
> + CFRelease(frame);
> + av_log_avfoundation_error(s, "error while creating audio
> converter", ret);
> return 1;
> }
> }
>
> - CFRelease(ctx->current_audio_frame);
> - ctx->current_audio_frame = nil;
> -
> - unlock_frames(ctx);
> + CFRelease(frame);
>
> return 0;
> }
> @@ -808,8 +806,6 @@ static int avf_read_header(AVFormatContext *s)
>
> ctx->num_video_devices = [devices count] + [devices_muxed count];
>
> - pthread_mutex_init(&ctx->frame_lock, NULL);
> -
> #if !TARGET_OS_IPHONE && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1070
> CGGetActiveDisplayList(0, NULL, &num_screens);
> #endif
> @@ -1010,6 +1006,21 @@ static int avf_read_header(AVFormatContext *s)
> // Initialize capture session
> ctx->capture_session = [[AVCaptureSession alloc] init];
>
> + OSStatus ret;
> + ret = CMSimpleQueueCreate(kCFAllocatorDefault, MAX_QUEUED_FRAMES,
> &ctx->video_frames_queue);
> +
> + if (ret != noErr) {
> + av_log_avfoundation_error(s, "error while creating frame
> queue", ret);
> + goto fail;
> + }
> +
> + ret = CMSimpleQueueCreate(kCFAllocatorDefault, MAX_QUEUED_FRAMES,
> &ctx->audio_frames_queue);
> +
> + if (ret != noErr) {
> + av_log_avfoundation_error(s, "error while creating frame
> queue", ret);
> + goto fail;
> + }
> +
> if (video_device && add_video_device(s, video_device)) {
> goto fail;
> }
> @@ -1039,7 +1050,8 @@ static int avf_read_header(AVFormatContext *s)
> fail:
> [pool release];
> destroy_context(ctx);
> - return AVERROR(EIO);
> + av_log(s, AV_LOG_ERROR, "Error while opening AVfoundation capture
> session\n");
> + return AVERROR_EXTERNAL;
> }
>
> static int copy_cvpixelbuffer(AVFormatContext *s,
> @@ -1088,38 +1100,35 @@ static int copy_cvpixelbuffer(AVFormatContext
> *s,
> static int avf_read_packet(AVFormatContext *s, AVPacket *pkt)
> {
> OSStatus ret;
> + int status;
> AVFContext* ctx = (AVFContext*)s->priv_data;
>
> do {
> - CVImageBufferRef image_buffer;
> - CMBlockBufferRef block_buffer;
> - lock_frames(ctx);
> -
> - if (ctx->current_frame != nil) {
> - int status;
> + if (1 <= CMSimpleQueueGetCount(ctx->video_frames_queue)) {
> int length = 0;
> -
> - image_buffer =
> CMSampleBufferGetImageBuffer(ctx->current_frame);
> - block_buffer =
> CMSampleBufferGetDataBuffer(ctx->current_frame);
> + CMSampleBufferRef video_frame =
> (CMSampleBufferRef)CMSimpleQueueDequeue(ctx->video_frames_queue);
> + CVImageBufferRef image_buffer =
> CMSampleBufferGetImageBuffer(video_frame);;
> + CMBlockBufferRef block_buffer =
> CMSampleBufferGetDataBuffer(video_frame);
>
> if (image_buffer != nil) {
> length = (int)CVPixelBufferGetDataSize(image_buffer);
> } else if (block_buffer != nil) {
> length =
> (int)CMBlockBufferGetDataLength(block_buffer);
> } else {
> - unlock_frames(ctx);
> + CFRelease(video_frame);
> return AVERROR(EINVAL);
> }
>
> - if (av_new_packet(pkt, length) < 0) {
> - unlock_frames(ctx);
> - return AVERROR(EIO);
> + status = av_new_packet(pkt, length);
> + if (status < 0) {
> + CFRelease(video_frame);
> + return status;
> }
>
> CMItemCount count;
> CMSampleTimingInfo timing_info;
>
> - if
> (CMSampleBufferGetOutputSampleTimingInfoArray(ctx->current_frame, 1,
> &timing_info, &count) == noErr) {
> + if
> (CMSampleBufferGetOutputSampleTimingInfoArray(video_frame, 1,
> &timing_info, &count) == noErr) {
> AVRational timebase_q = av_make_q(1,
> timing_info.presentationTimeStamp.timescale);
> pkt->pts = pkt->dts =
> av_rescale_q(timing_info.presentationTimeStamp.value, timebase_q,
> avf_time_base_q);
> }
> @@ -1133,18 +1142,18 @@ static int avf_read_packet(AVFormatContext *s,
> AVPacket *pkt)
> status = 0;
> ret = CMBlockBufferCopyDataBytes(block_buffer, 0,
> pkt->size, pkt->data);
> if (ret != kCMBlockBufferNoErr) {
> - status = AVERROR(EIO);
> + av_log_avfoundation_error(s, "error while copying
> buffer data", ret);
> + status = AVERROR_EXTERNAL;
> }
> }
> - CFRelease(ctx->current_frame);
> - ctx->current_frame = nil;
> + CFRelease(video_frame);
>
> if (status < 0) {
> - unlock_frames(ctx);
> return status;
> }
> - } else if (ctx->current_audio_frame != nil) {
> - CMBlockBufferRef block_buffer =
> CMSampleBufferGetDataBuffer(ctx->current_audio_frame);
> + } else if (1 <=
> CMSimpleQueueGetCount(ctx->audio_frames_queue)) {
> + CMSampleBufferRef audio_frame =
> (CMSampleBufferRef)CMSimpleQueueDequeue(ctx->audio_frames_queue);
> + CMBlockBufferRef block_buffer =
> CMSampleBufferGetDataBuffer(audio_frame);
>
> size_t input_size =
> CMBlockBufferGetDataLength(block_buffer);
> int buffer_size = input_size / ctx->audio_buffers;
> @@ -1170,8 +1179,9 @@ static int avf_read_packet(AVFormatContext *s,
> AVPacket *pkt)
>
> if (ret != kCMBlockBufferNoErr) {
> av_free(input_buffer);
> - unlock_frames(ctx);
> - return AVERROR(EIO);
> + CFRelease(audio_frame);
> + av_log_avfoundation_error(s, "error while
> accessing audio buffer data", ret);
> + return AVERROR_EXTERNAL;
> }
> }
>
> @@ -1188,23 +1198,25 @@ static int avf_read_packet(AVFormatContext *s,
> AVPacket *pkt)
> av_free(input_buffer);
>
> if (ret != noErr) {
> - unlock_frames(ctx);
> - return AVERROR(EIO);
> + CFRelease(audio_frame);
> + av_log_avfoundation_error(s, "error while
> converting audio data", ret);
> + return AVERROR_EXTERNAL;
> }
>
> pkt->size = output_buffer.mBuffers[0].mDataByteSize;
> } else {
> ret = CMBlockBufferCopyDataBytes(block_buffer, 0,
> pkt->size, pkt->data);
> if (ret != kCMBlockBufferNoErr) {
> - unlock_frames(ctx);
> - return AVERROR(EIO);
> + CFRelease(audio_frame);
> + av_log_avfoundation_error(s, "error while
> copying audio data", ret);
> + return AVERROR_EXTERNAL;
> }
> }
>
> CMItemCount count;
> CMSampleTimingInfo timing_info;
>
> - if
> (CMSampleBufferGetOutputSampleTimingInfoArray(ctx->current_audio_frame,
> 1, &timing_info, &count) == noErr) {
> + if
> (CMSampleBufferGetOutputSampleTimingInfoArray(audio_frame, 1,
> &timing_info, &count) == noErr) {
> AVRational timebase_q = av_make_q(1,
> timing_info.presentationTimeStamp.timescale);
> pkt->pts = pkt->dts =
> av_rescale_q(timing_info.presentationTimeStamp.value, timebase_q,
> avf_time_base_q);
> }
> @@ -1212,21 +1224,15 @@ static int avf_read_packet(AVFormatContext *s,
> AVPacket *pkt)
> pkt->stream_index = ctx->audio_stream_index;
> pkt->flags |= AV_PKT_FLAG_KEY;
>
> - CFRelease(ctx->current_audio_frame);
> - ctx->current_audio_frame = nil;
> -
> - unlock_frames(ctx);
> + CFRelease(audio_frame);
> } else {
> pkt->data = NULL;
> - unlock_frames(ctx);
> if (ctx->observed_quit) {
> return AVERROR_EOF;
> } else {
> return AVERROR(EAGAIN);
> }
> }
> -
> - unlock_frames(ctx);
> } while (!pkt->data);
>
> return 0;
> --
> 2.32.0 (Apple Git-132)
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list