[FFmpeg-devel] [PATCH v7] Improved the performance of 1 decode + N filter graphs and adaptive bitrate.

[Mark Thompson]

On 21/03/2019 15:09, Shaofei Wang wrote:
> It enabled MULTIPLE SIMPLE filter graph concurrency, which bring above about
> 4%~20% improvement in some 1:N scenarios by CPU or GPU acceleration
> 
> ...> 
> Signed-off-by: Wang, Shaofei <shaofei.wang at intel.com>
> Reviewed-by: Michael Niedermayer <michael at niedermayer.cc>
> Reviewed-by: Mark Thompson <sw at jkqxz.net>

The reviewed-by annotation generally implies review approval for a patch, which I don't think is true for either of the stated people.

> ---
> The patch will only effect on multiple SIMPLE filter graphs pipeline,
> Passed fate and refine the possible data race,
> AFL tested, without introducing extra crashs/hangs:
> 
> ...
> 
>  fftools/ffmpeg.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++------
>  fftools/ffmpeg.h |  13 +++++
>  2 files changed, 169 insertions(+), 16 deletions(-)> 
> ...
> @@ -1419,12 +1436,13 @@ static void finish_output_stream(OutputStream *ost)
>   *
>   * @return  0 for success, <0 for severe errors
>   */
> -static int reap_filters(int flush)
> +static int reap_filters(int flush, InputFilter * ifilter)
>  {
>      AVFrame *filtered_frame = NULL;
>      int i;
>  
> -    /* Reap all buffers present in the buffer sinks */
> +    /* Reap all buffers present in the buffer sinks or just reap specified
> +     * buffer which related with the filter graph who got ifilter as input*/
>      for (i = 0; i < nb_output_streams; i++) {
>          OutputStream *ost = output_streams[i];
>          OutputFile    *of = output_files[ost->file_index];
> @@ -1432,13 +1450,25 @@ static int reap_filters(int flush)
>          AVCodecContext *enc = ost->enc_ctx;
>          int ret = 0;
>  
> +        if (ifilter && abr_threads_enabled)
> +            if (ost != ifilter->graph->outputs[0])
> +                continue;

This comparison is always false.

src/fftools/ffmpeg.c: In function ‘reap_filters’:
src/fftools/ffmpeg.c:1457:21: warning: comparison of distinct pointer types lacks a cast
             if (ost != ifilter->graph->outputs[0])
                     ^~

> ...

The output is entirely serialised in this patch because of it, which I don't think was intended.  Doing that certainly avoid data races, but much of the locking is redundant and the improvement numbers stated in the commit message for earlier versions do not apply.

- Mark