Re: [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@gmail.com>]

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@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".