[FFmpeg-devel] [PATCH 1/6] cbs_h2645: Unify the free-functions via macro

[Mark Thompson]

On 09/11/18 05:31, Andreas Rheinhardt wrote:
> The similarity between several free-functions is exploited to create
> them via a common macro.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at googlemail.com>
> ---
>  libavcodec/cbs_h2645.c | 55 ++++++++++++------------------------------
>  1 file changed, 15 insertions(+), 40 deletions(-)
> 
> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
> index e55bd00183..37b0207420 100644
> --- a/libavcodec/cbs_h2645.c
> +++ b/libavcodec/cbs_h2645.c
> @@ -414,13 +414,22 @@ static int cbs_h2645_read_more_rbsp_data(GetBitContext *gbc)
>  #undef allocate
>  
>  
> -static void cbs_h264_free_pps(void *unit, uint8_t *content)
> -{
> -    H264RawPPS *pps = (H264RawPPS*)content;
> -    av_buffer_unref(&pps->slice_group_id_ref);
> -    av_freep(&content);
> +#define cbs_h2645_free(h26n, name, var, buffer) \
> +static void cbs_h26 ## h26n ## _free_ ## var(void *unit, uint8_t *content) \
> +{ \
> +    H26 ## h26n ## Raw ## name *var = (H26 ## h26n ## Raw ## name*)content; \
> +    av_buffer_unref(&var->buffer ## _ref); \
> +    av_freep(&content); \
>  }
>  
> +cbs_h2645_free(4, PPS, pps, slice_group_id)
> +cbs_h2645_free(5, VPS, vps, extension_data.data)
> +cbs_h2645_free(5, SPS, sps, extension_data.data)
> +cbs_h2645_free(5, PPS, pps, extension_data.data)
> +
> +cbs_h2645_free(4, Slice, slice, data)
> +cbs_h2645_free(5, Slice, slice, data)
> +
>  static void cbs_h264_free_sei_payload(H264RawSEIPayload *payload)
>  {
>      switch (payload->payload_type) {
> @@ -452,41 +461,6 @@ static void cbs_h264_free_sei(void *unit, uint8_t *content)
>      av_freep(&content);
>  }
>  
> -static void cbs_h264_free_slice(void *unit, uint8_t *content)
> -{
> -    H264RawSlice *slice = (H264RawSlice*)content;
> -    av_buffer_unref(&slice->data_ref);
> -    av_freep(&content);
> -}
> -
> -static void cbs_h265_free_vps(void *unit, uint8_t *content)
> -{
> -    H265RawVPS *vps = (H265RawVPS*)content;
> -    av_buffer_unref(&vps->extension_data.data_ref);
> -    av_freep(&content);
> -}
> -
> -static void cbs_h265_free_sps(void *unit, uint8_t *content)
> -{
> -    H265RawSPS *sps = (H265RawSPS*)content;
> -    av_buffer_unref(&sps->extension_data.data_ref);
> -    av_freep(&content);
> -}
> -
> -static void cbs_h265_free_pps(void *unit, uint8_t *content)
> -{
> -    H265RawPPS *pps = (H265RawPPS*)content;
> -    av_buffer_unref(&pps->extension_data.data_ref);
> -    av_freep(&content);
> -}
> -
> -static void cbs_h265_free_slice(void *unit, uint8_t *content)
> -{
> -    H265RawSlice *slice = (H265RawSlice*)content;
> -    av_buffer_unref(&slice->data_ref);
> -    av_freep(&content);
> -}
> -
>  static void cbs_h265_free_sei_payload(H265RawSEIPayload *payload)
>  {
>      switch (payload->payload_type) {
> @@ -727,6 +701,7 @@ static int cbs_h26 ## h26n ## _replace_ ## ps_var(CodedBitstreamContext *ctx, \
>      return 0; \
>  }
>  
> +
>  cbs_h2645_replace_ps(4, SPS, sps, seq_parameter_set_id)
>  cbs_h2645_replace_ps(4, PPS, pps, pic_parameter_set_id)
>  cbs_h2645_replace_ps(5, VPS, vps, vps_video_parameter_set_id)
> 

I'm not convinced that this is an improvement.  Those functions were previously simple and clear, while now they are a bit more obscure and I'm not sure you've gained anything except a slight saving in source code size.  The similarity between these functions that you're exploiting is that they free exactly one internal buffer, but otherwise they don't really have very much in common (unlike the replace_ps case).

If the aim is to avoid a bit of boilerplate, perhaps we could do something like:

#define cbs_h2645_free_unit(h26n, name, var, code) \
static void cbs_h26 ## h26n ## _free_ ## var(void *unit, uint8_t *content) \
{ \
    H26 ## h26n ## Raw ## name *var = (H26 ## h26n ## Raw ## name*)content; \
    do code while (0);
    av_freep(&content); \
}

cbs_h2645_free_unit(4, PPS,   pps,   { av_buffer_unref(&pps->slice_group_id); })
cbs_h2645_free_unit(4, Slice, slice, { av_buffer_unref(&slice->extension_data.data_ref); })
cbs_h2645_free_unit(4, SEI,   sei,   {
    int i;
    for (i = 0; i < sei->payload_count; i++)
        cbs_h264_free_sei_payload(&sei->payload[i]);
})

to exploit the commonality without hiding anything?  (Not necessarily exactly this, but something along those lines so the buffer behaviour is more clear than your version above.)

- Mark