[FFmpeg-devel] avcodec/cbs_mpeg2: use existing buffer reference if available when splitting fragments

Submitted by James Almer on April 24, 2018, 2:17 a.m.

Details

Message ID 20180424021700.11720-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer April 24, 2018, 2:17 a.m.
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/cbs_mpeg2.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Comments

James Almer April 24, 2018, 10:22 p.m.
On 4/23/2018 11:17 PM, James Almer wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/cbs_mpeg2.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
> index bfb64a0851..086d08ed64 100644
> --- a/libavcodec/cbs_mpeg2.c
> +++ b/libavcodec/cbs_mpeg2.c
> @@ -146,14 +146,17 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
>              unit_size = (end - 4) - (start - 1);
>          }
>  
> -        unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE);
> -        if (!unit_data)
> -            return AVERROR(ENOMEM);
> -        memcpy(unit_data, start - 1, unit_size);
> -        memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
> +        if (header) {
> +            unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE);
> +            if (!unit_data)
> +                return AVERROR(ENOMEM);
> +            memcpy(unit_data, start - 1, unit_size);
> +            memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
> +        } else
> +            unit_data = (uint8_t *)start - 1;
>  
>          err = ff_cbs_insert_unit_data(ctx, frag, i, unit_type,
> -                                      unit_data, unit_size, NULL);
> +                                      unit_data, unit_size, frag->data_ref);
>          if (err < 0) {
>              av_freep(&unit_data);
>              return err;

The alternative is to make ff_cbs_read_extradata() create an AVBufferRef
with the contents of par->extradata, so every
CodedBitstreamType->split_fragment() function can safely assume the
fragment will always be reference counted.

I think that would be cleaner overall.
James Almer April 25, 2018, 2:58 a.m.
On 4/24/2018 7:22 PM, James Almer wrote:
> On 4/23/2018 11:17 PM, James Almer wrote:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavcodec/cbs_mpeg2.c | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
>> index bfb64a0851..086d08ed64 100644
>> --- a/libavcodec/cbs_mpeg2.c
>> +++ b/libavcodec/cbs_mpeg2.c
>> @@ -146,14 +146,17 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
>>              unit_size = (end - 4) - (start - 1);
>>          }
>>  
>> -        unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE);
>> -        if (!unit_data)
>> -            return AVERROR(ENOMEM);
>> -        memcpy(unit_data, start - 1, unit_size);
>> -        memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>> +        if (header) {
>> +            unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE);
>> +            if (!unit_data)
>> +                return AVERROR(ENOMEM);
>> +            memcpy(unit_data, start - 1, unit_size);
>> +            memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>> +        } else
>> +            unit_data = (uint8_t *)start - 1;
>>  
>>          err = ff_cbs_insert_unit_data(ctx, frag, i, unit_type,
>> -                                      unit_data, unit_size, NULL);
>> +                                      unit_data, unit_size, frag->data_ref);
>>          if (err < 0) {
>>              av_freep(&unit_data);

And this is obviously going to crash if header == 0...

I'll wait until the suggestion below is accepted or not before sending a
fixed version.

>>              return err;
> 
> The alternative is to make ff_cbs_read_extradata() create an AVBufferRef
> with the contents of par->extradata, so every
> CodedBitstreamType->split_fragment() function can safely assume the
> fragment will always be reference counted.
> 
> I think that would be cleaner overall.
>
Mark Thompson April 25, 2018, 9:36 p.m.
On 24/04/18 23:22, James Almer wrote:
> On 4/23/2018 11:17 PM, James Almer wrote:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavcodec/cbs_mpeg2.c | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
>> index bfb64a0851..086d08ed64 100644
>> --- a/libavcodec/cbs_mpeg2.c
>> +++ b/libavcodec/cbs_mpeg2.c
>> @@ -146,14 +146,17 @@ static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
>>              unit_size = (end - 4) - (start - 1);
>>          }
>>  
>> -        unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE);
>> -        if (!unit_data)
>> -            return AVERROR(ENOMEM);
>> -        memcpy(unit_data, start - 1, unit_size);
>> -        memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>> +        if (header) {
>> +            unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE);
>> +            if (!unit_data)
>> +                return AVERROR(ENOMEM);
>> +            memcpy(unit_data, start - 1, unit_size);
>> +            memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>> +        } else
>> +            unit_data = (uint8_t *)start - 1;
>>  
>>          err = ff_cbs_insert_unit_data(ctx, frag, i, unit_type,
>> -                                      unit_data, unit_size, NULL);
>> +                                      unit_data, unit_size, frag->data_ref);
>>          if (err < 0) {
>>              av_freep(&unit_data);
>>              return err;
> 
> The alternative is to make ff_cbs_read_extradata() create an AVBufferRef
> with the contents of par->extradata, so every
> CodedBitstreamType->split_fragment() function can safely assume the
> fragment will always be reference counted.
> 
> I think that would be cleaner overall.

I agree, it would be much cleaner if we can assume that the input fragment is always refcounted.  (The AV1 code here assumes it already too, though we didn't have extradata for it at the time.)

- Mark

Patch hide | download patch | download mbox

diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
index bfb64a0851..086d08ed64 100644
--- a/libavcodec/cbs_mpeg2.c
+++ b/libavcodec/cbs_mpeg2.c
@@ -146,14 +146,17 @@  static int cbs_mpeg2_split_fragment(CodedBitstreamContext *ctx,
             unit_size = (end - 4) - (start - 1);
         }
 
-        unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE);
-        if (!unit_data)
-            return AVERROR(ENOMEM);
-        memcpy(unit_data, start - 1, unit_size);
-        memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
+        if (header) {
+            unit_data = av_malloc(unit_size + AV_INPUT_BUFFER_PADDING_SIZE);
+            if (!unit_data)
+                return AVERROR(ENOMEM);
+            memcpy(unit_data, start - 1, unit_size);
+            memset(unit_data + unit_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
+        } else
+            unit_data = (uint8_t *)start - 1;
 
         err = ff_cbs_insert_unit_data(ctx, frag, i, unit_type,
-                                      unit_data, unit_size, NULL);
+                                      unit_data, unit_size, frag->data_ref);
         if (err < 0) {
             av_freep(&unit_data);
             return err;