[FFmpeg-devel] [PATCH 1/2] avcodec/cbs_h2645: use simple data buffers for some parameter set extensions

[James Almer]

On 5/8/2018 7:17 PM, Mark Thompson wrote:
> On 08/05/18 21:48, James Almer wrote:
>> There's no gain from using AVBufferRef for these, as no copies or
>> references are ever made.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> There is however a raw copy of the struct storing these buffers,
>> which is dangerous and fragile.
>> This patch is in preparation to change how the above is handled.
>>
>>  libavcodec/cbs_h264.h  |  1 -
>>  libavcodec/cbs_h2645.c | 13 ++++++-------
>>  libavcodec/cbs_h265.h  |  1 -
>>  3 files changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h
>> index 2219d9da8d..becea3adfe 100644
>> --- a/libavcodec/cbs_h264.h
>> +++ b/libavcodec/cbs_h264.h
>> @@ -197,7 +197,6 @@ typedef struct H264RawPPS {
>>      uint16_t pic_size_in_map_units_minus1;
>>  
>>      uint8_t *slice_group_id;
>> -    AVBufferRef *slice_group_id_ref;
>>  
>>      uint8_t num_ref_idx_l0_default_active_minus1;
>>      uint8_t num_ref_idx_l1_default_active_minus1;
>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>> index 64a1a2d1ee..580ca09f63 100644
>> --- a/libavcodec/cbs_h2645.c
>> +++ b/libavcodec/cbs_h2645.c
>> @@ -318,10 +318,9 @@ static int cbs_h2645_read_more_rbsp_data(GetBitContext *gbc)
>>  #define byte_alignment(rw) (get_bits_count(rw) % 8)
>>  
>>  #define allocate(name, size) do { \
>> -        name ## _ref = av_buffer_allocz(size); \
>> -        if (!name ## _ref) \
>> +        name = av_mallocz(size); \
>> +        if (!name) \
>>              return AVERROR(ENOMEM); \
>> -        name = name ## _ref->data; \
>>      } while (0)
> 
> This breaks other users of this macro (H.264 SEI).
> 
> The reason for using the bufferref here is not really that you might want to make more references to it.  Rather, it is for the alloc/free properties which give control to the user - for example, they can set one of these pointers to some internal static buffer they hold while setting _ref to null, and the free code still does the right thing (i.e. nothing).
> 
> I don't think that argument will necessarily apply to any of the values changed here - I doubt anyone will ever touch the FMO slice_group_id, and the H.265 PS extension data bits will need to have defined meanings (and therefore moved out of the "unknown future stuff" case) before they gets used.  Still, I'm not sure how you've gained anything - since the PS objects are refcounted in the following patch, how does this change actually help?

Removing the overhead of having these be AVBufferRef instead of a simple
malloc'ed array, since at least after a quick glance i couldn't find any
reason for it.

If you think keeping them as AVBufferRef is better/future proof then we
can drop this. The next patch works around the issue of memcpy'ing them
anyway.

> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>