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.w...@intel.com>
> Reviewed-by: Michael Niedermayer <mich...@niedermayer.cc>
> Reviewed-by: Mark Thompson <s...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

Reply via email to