[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

[Romain Beauxis]

Le mer. 5 janv. 2022 à 08:50, Marvin Scholz <epirat07 at gmail.com> a écrit :
>
> 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?

Done!

> > +     [[[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.

That's right, thanks for pointing that out. Just sent a v9 version of
the patchset fixing that and also dropping the log level for the error
returned when the queue is full. In practice, the queue can become
full pretty often, for instance when waiting on a remote output
connection so these logs end up polluting the output in situations
that are not really problematic.

Thanks for looking into this!
-- Romain