Re: [FFmpeg-devel] [PATCH 27/42] avcodec/pthread_frame: Add new progress API

[Anton Khirnov <anton@khirnov.net>]

Quoting Andreas Rheinhardt (2023-09-19 21:57:19)
> Frame-threaded decoders with inter-frame dependencies
> use the ThreadFrame API for syncing. It works as follows:
> 
> During init each thread allocates an AVFrame for every
> ThreadFrame.
> 
> Thread A reads the header of its packet and allocates
> a buffer for an AVFrame with ff_thread_get_ext_buffer()
> (which also allocates a small structure that is shared
> with other references to this frame) and sets its fields,
> including side data. Then said thread calls ff_thread_finish_setup().
> From that moment onward it is not allowed to change any
> of the AVFrame fields at all any more, but it may change
> fields which are an indirection away, like the content of
> AVFrame.data or already existing side data.
> 
> After thread A has called ff_thread_finish_setup(),
> another thread (the user one) calls the codec's update_thread_context
> callback which in turn calls ff_thread_ref_frame() which
> calls av_frame_ref() which reads every field of A's
> AVFrame; hence the above restriction on modifications
> of the AVFrame (as any modification of the AVFrame by A after
> ff_thread_finish_setup() would be a data race). Of course,
> this av_frame_ref() also incurs allocations and therefore
> needs to be checked. ff_thread_ref_frame() also references
> the small structure used for communicating progress.
> 
> This av_frame_ref() makes it awkward to propagate values that
> only become known during decoding to later threads (in case of
> frame reordering or other mechanisms of delayed output (like
> show-existing-frames) it's not the decoding thread, but a later
> thread that returns the AVFrame). E.g. for VP9 when exporting video
> encoding parameters as side data the number of blocks only
> becomes known during decoding, so one can't allocate the side data
> before ff_thread_finish_setup(). It is currently being done afterwards
> and this leads to a data race in the vp9-encparams test when using
> frame-threading. Returning decode_error_flags is also complicated
> by this.
> 
> To perform this exchange a buffer shared between the references
> is needed (notice that simply giving the later threads a pointer
> to the original AVFrame does not work, because said AVFrame will
> be reused lateron when thread A decodes the next packet given to it).
> One could extend the buffer already used for progress for this
> or use a new one (requiring yet another allocation), yet both
> of these approaches have the drawback of being unnatural, ugly
> and requiring quite a lot of ad-hoc code. E.g. in case of the VP9
> side data mentioned above one could not simply use the helper
> that allocates and adds the side data to an AVFrame in one go.
> 
> The ProgressFrame API meanwhile offers a different solution to all
> of this. It is based around the idea that the most natural
> shared object for sharing information about an AVFrame between
> decoding threads is the AVFrame itself. To actually implement this
> the AVFrame needs to be reference counted. This is achieved by
> putting a (ownership) pointer into a shared (and opaque) structure
> that is managed by the RefStruct API and which also contains
> the stuff necessary for progress reporting.

Do we really need an owner? I never liked this notion for ThreadFrames.
Might as well make the mutex and the cond per-ProgressFrame and drop the
debug logs - I never found them useful. Then this API could be entirely
independent of the frame threading implementation and could potentially
be used elsewhere.

> +#ifndef AVCODEC_PROGRESSFRAME_H
> +#define AVCODEC_PROGRESSFRAME_H
> +
> +/**
> + * ProgressFrame is an API to easily share frames without an underlying
> + * av_frame_ref(). Its main usecase is in frame-threading scenarios,
> + * yet it could also be used for purely single-threaded decoders that
> + * want to keep multiple references to the same frame.

nit: the name is not ideal for these other use cases. You could call it
     something like SharedFrame instead.

> +typedef struct ProgressFrame {
> +    struct AVFrame *f;
> +    struct ProgressInternal *progress;
> +} ProgressFrame;
> +
> +/**
> + * Notify later decoding threads when part of their reference picture is ready.

s/picture/frame/ everywhere

"picture" has a special meaning for interlaced video and might confuse
readers.

> diff --git a/libavcodec/progressframe_internal.h b/libavcodec/progressframe_internal.h
> new file mode 100644
> index 0000000000..1101207106
> --- /dev/null
> +++ b/libavcodec/progressframe_internal.h
> @@ -0,0 +1,32 @@
> +/*
> + * Copyright (c) 2022 Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#ifndef AVCODEC_PROGRESSFRAME_INTERNAL_H
> +#define AVCODEC_PROGRESSFRAME_INTERNAL_H

Mention who this header is for, so clue-free decoder writers don't
start including it.

> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index 282f3fad58..9e827f0606 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -34,6 +34,8 @@
>  #include "hwaccel_internal.h"
>  #include "hwconfig.h"
>  #include "internal.h"
> +#include "progressframe.h"
> +#include "progressframe_internal.h"
>  #include "pthread_internal.h"
>  #include "refstruct.h"
>  #include "thread.h"
> @@ -795,6 +797,7 @@ static av_cold int init_thread(PerThreadContext *p, int *threads_to_free,
>      if (!copy->internal)
>          return AVERROR(ENOMEM);
>      copy->internal->thread_ctx = p;
> +    copy->internal->progress_frame_pool = avctx->internal->progress_frame_pool;

It feels cleaner to me to make each of those a separate reference,
especially since it's pretty much free.

-- 
Anton Khirnov
_______________________________________________
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".