[FFmpeg-devel] [PATCH] avfilter/ccfifo: remove unnecessary context allocations

[Leo Izen]

On 5/11/23 14:03, James Almer wrote:
> This is not public API, no it has no need for an alloc() and free()
> functions. The struct can reside on stack.
> 
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
>   libavfilter/ccfifo.c        | 49 ++++++++++++-------------------------
>   libavfilter/ccfifo.h        | 42 ++++++++++++++++++-------------
>   libavfilter/tinterlace.h    |  2 +-
>   libavfilter/vf_bwdif.c      |  8 +++---
>   libavfilter/vf_ccrepack.c   | 13 +++++-----
>   libavfilter/vf_fps.c        | 13 +++++-----
>   libavfilter/vf_tinterlace.c | 18 +++++++-------
>   libavfilter/vf_yadif.c      |  8 +++---
>   libavfilter/vf_yadif_cuda.c |  6 ++---
>   libavfilter/yadif.h         |  2 +-
>   libavfilter/yadif_common.c  |  6 ++---
>   11 files changed, 81 insertions(+), 86 deletions(-)
> 
> diff --git a/libavfilter/ccfifo.c b/libavfilter/ccfifo.c
> index 5fb68ce04c..a3c6a09698 100644
> --- a/libavfilter/ccfifo.c
> +++ b/libavfilter/ccfifo.c
> @@ -23,18 +23,6 @@
>   
>   #include "ccfifo.h"
>   
> -struct AVCCFifo {
> -    AVFifo *cc_608_fifo;
> -    AVFifo *cc_708_fifo;
> -    AVRational framerate;
> -    int expected_cc_count;
> -    int expected_608;
> -    int cc_detected;
> -    int passthrough;
> -    int passthrough_warning;
> -    void *log_ctx;
> -};
> -
>   #define MAX_CC_ELEMENTS 128
>   #define CC_BYTES_PER_ENTRY 3
>   
> @@ -55,25 +43,18 @@ const static struct cc_lookup cc_lookup_vals[] = {
>       { 60000, 1001, 10, 1},
>   };
>   
> -void ff_ccfifo_freep(AVCCFifo **ccf)
> +void ff_ccfifo_uninit(CCFifo *ccf)
>   {
> -    AVCCFifo *tmp = *ccf;
> -    if (tmp) {
> -        av_fifo_freep2(&tmp->cc_608_fifo);
> -        av_fifo_freep2(&tmp->cc_708_fifo);
> -    }
> -    av_freep(ccf);
> +    av_fifo_freep2(&ccf->cc_608_fifo);
> +    av_fifo_freep2(&ccf->cc_708_fifo);
> +    memset(&ccf, 0, sizeof(ccf));

See later comment about memset, I have the same question. ccf is a pointer.

>   }
>   
> -AVCCFifo *ff_ccfifo_alloc(AVRational framerate, void *log_ctx)
> +int ff_ccfifo_init(CCFifo *ccf, AVRational framerate, void *log_ctx)
>   {
> -    AVCCFifo *ccf;
>       int i;
>   
> -    ccf = av_mallocz(sizeof(*ccf));
> -    if (!ccf)
> -        return NULL;
> -
> +    memset(&ccf, 0, sizeof(ccf));
>       ccf->log_ctx = log_ctx;
>       ccf->framerate = framerate;

I'm confused by this memset, why isn't it: memset(ccf, 0, 
sizeof(CCFifo)); As written it appears to be equivalent to: ccf = NULL;

Unless I'm missing something here, this should immediately pop a null 
dereference.


- Leo Izen (Traneptora / thebombzen)