[FFmpeg-devel] [PATCH 2/2] avcodec/cbs_h2645: create a reference to the existing buffer when decomposing slice units

[Mark Thompson]

On 28/04/18 15:52, James Almer wrote:
> On 4/28/2018 5:02 AM, Xiang, Haihao wrote:
>> On Fri, 2018-04-27 at 20:50 -0300, James Almer wrote:
>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>> ---
>>>  libavcodec/cbs_h2645.c | 18 ++++--------------
>>>  1 file changed, 4 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>>> index 5585831cf6..5e5598f377 100644
>>> --- a/libavcodec/cbs_h2645.c
>>> +++ b/libavcodec/cbs_h2645.c
>>> @@ -776,15 +776,10 @@ static int cbs_h264_read_nal_unit(CodedBitstreamContext
>>> *ctx,
>>>              }
>>>  
>>>              slice->data_size = len - pos / 8;
>>> -            slice->data_ref  = av_buffer_alloc(slice->data_size +
>>> -                                               AV_INPUT_BUFFER_PADDING_SIZE);
>>> +            slice->data_ref  = av_buffer_ref(unit->data_ref);
>>
>> According the comment for CodedBitstreamUnit::data_ref, unit->data_ref might be
>> NULL, how about adding 'av_assert0(unit->data_ref)' before the above line? 
> 
> Judging by Mark's cbs_jpeg patch, which i notice now is doing the same
> thing (so my patches here were probably in his local queue in some for
> anyway), i guess it's safe to assume unit->data_ref will never be null.
> All the functions allocating a unit so far (ff_cbs_insert_unit_data,
> ff_cbs_alloc_unit_data) seem to make sure it's reference counted.
> 
> I'll wait for Mark to comment in any case.

Yeah, I think the conclusion from recent changes is that the fragment data must always be reference-counted.

I'll send a patch to fix the documentation and add some asserts to make sure that is indeed always the case.

Thanks,

- Mark

>>>              if (!slice->data_ref)
>>>                  return AVERROR(ENOMEM);
>>> -            slice->data = slice->data_ref->data;
>>> -            memcpy(slice->data,
>>> -                   unit->data + pos / 8, slice->data_size);
>>> -            memset(slice->data + slice->data_size, 0,
>>> -                   AV_INPUT_BUFFER_PADDING_SIZE);
>>> +            slice->data = unit->data + pos / 8;
>>>              slice->data_bit_start = pos % 8;
>>>          }
>>>          break;
>>> @@ -946,15 +941,10 @@ static int cbs_h265_read_nal_unit(CodedBitstreamContext
>>> *ctx,
>>>              }
>>>  
>>>              slice->data_size = len - pos / 8;
>>> -            slice->data_ref  = av_buffer_alloc(slice->data_size +
>>> -                                               AV_INPUT_BUFFER_PADDING_SIZE);
>>> +            slice->data_ref  = av_buffer_ref(unit->data_ref);
>>
>> Same comment as above. 
>>
>>>              if (!slice->data_ref)
>>>                  return AVERROR(ENOMEM);
>>> -            slice->data = slice->data_ref->data;
>>> -            memcpy(slice->data,
>>> -                   unit->data + pos / 8, slice->data_size);
>>> -            memset(slice->data + slice->data_size, 0,
>>> -                   AV_INPUT_BUFFER_PADDING_SIZE);
>>> +            slice->data = unit->data + pos / 8;
>>>              slice->data_bit_start = pos % 8;
>>>          }
>>>          break;