diff mbox

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

Message ID 20180427235000.6544-1-jamrial@gmail.com
State Accepted
Commit 0807a771600956e1f3e36a202fd3877418541eb0
Headers show

Commit Message

James Almer April 27, 2018, 11:50 p.m. UTC 
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/cbs_h2645.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

Comments

Xiang, Haihao April 28, 2018, 8:02 a.m. UTC | #1 
On Fri, 2018-04-27 at 20:50 -0300, James Almer wrote:
> Signed-off-by: James Almer <jamrial@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? 


>              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;
James Almer April 28, 2018, 2:52 p.m. UTC | #2 
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@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.

> 
> 
>>              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;
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Mark Thompson April 30, 2018, 11:08 p.m. UTC | #3 
On 28/04/18 00:50, James Almer wrote:
> Signed-off-by: James Almer <jamrial@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);
>              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);
>              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;
> 

Also looks good.

(We could probably avoid duplicating this code, too, though that's orthogonal to the patch.)

Thanks,

- Mark
Mark Thompson April 30, 2018, 11:11 p.m. UTC | #4 
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@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;
diff mbox

Patch

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);
             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);
             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;