[FFmpeg-devel,1/6] cbs: Fragment/unit data is always reference counted

Submitted by Mark Thompson on April 30, 2018, 11:26 p.m.

Details

Message ID 20180430232605.4846-1-sw@jkqxz.net
State New
Headers show

Commit Message

Mark Thompson April 30, 2018, 11:26 p.m.
Make this clear in the documentation and add some asserts to ensure
that it is always true.
---
As suggested earlier :)


 libavcodec/cbs.c | 18 ++++++++++++------
 libavcodec/cbs.h | 10 ++++++----
 2 files changed, 18 insertions(+), 10 deletions(-)

Comments

James Almer May 1, 2018, 3:51 p.m.
On 4/30/2018 8:26 PM, Mark Thompson wrote:
> Make this clear in the documentation and add some asserts to ensure
> that it is always true.
> ---
> As suggested earlier :)
> 
> 
>  libavcodec/cbs.c | 18 ++++++++++++------
>  libavcodec/cbs.h | 10 ++++++----
>  2 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index d81b4e03f7..0d6ffe8824 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -140,26 +140,30 @@ static int cbs_read_fragment_content(CodedBitstreamContext *ctx,
>      int err, i, j;
>  
>      for (i = 0; i < frag->nb_units; i++) {
> +        CodedBitstreamUnit *unit = &frag->units[i];
> +
>          if (ctx->decompose_unit_types) {
>              for (j = 0; j < ctx->nb_decompose_unit_types; j++) {
> -                if (ctx->decompose_unit_types[j] == frag->units[i].type)
> +                if (ctx->decompose_unit_types[j] == unit->type)
>                      break;
>              }
>              if (j >= ctx->nb_decompose_unit_types)
>                  continue;
>          }
>  
> -        av_buffer_unref(&frag->units[i].content_ref);
> -        frag->units[i].content = NULL;
> +        av_buffer_unref(&unit->content_ref);
> +        unit->content = NULL;
> +
> +        av_assert0(unit->data && unit->data_ref);
>  
> -        err = ctx->codec->read_unit(ctx, &frag->units[i]);
> +        err = ctx->codec->read_unit(ctx, unit);
>          if (err == AVERROR(ENOSYS)) {
>              av_log(ctx->log_ctx, AV_LOG_VERBOSE,
>                     "Decomposition unimplemented for unit %d "
> -                   "(type %"PRIu32").\n", i, frag->units[i].type);
> +                   "(type %"PRIu32").\n", i, unit->type);
>          } else if (err < 0) {
>              av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to read unit %d "
> -                   "(type %"PRIu32").\n", i, frag->units[i].type);
> +                   "(type %"PRIu32").\n", i, unit->type);
>              return err;
>          }
>      }
> @@ -277,6 +281,7 @@ int ff_cbs_write_fragment_data(CodedBitstreamContext *ctx,
>                     "(type %"PRIu32").\n", i, unit->type);
>              return err;
>          }
> +        av_assert0(unit->data && unit->data_ref);
>      }
>  
>      av_buffer_unref(&frag->data_ref);
> @@ -287,6 +292,7 @@ int ff_cbs_write_fragment_data(CodedBitstreamContext *ctx,
>          av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to assemble fragment.\n");
>          return err;
>      }
> +    av_assert0(frag->data && frag->data_ref);

You can remove the assert in ff_cbs_write_packet() if you add this one.

>  
>      return 0;
>  }
> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
> index 402eb39e00..487358afaf 100644
> --- a/libavcodec/cbs.h
> +++ b/libavcodec/cbs.h
> @@ -84,8 +84,9 @@ typedef struct CodedBitstreamUnit {
>       */
>      size_t   data_bit_padding;
>      /**
> -     * If data is reference counted, a reference to the buffer containing
> -     * data.  Null if data is not reference counted.
> +     * A reference to the buffer containing data.
> +     *
> +     * Must be set if data is not NULL.
>       */
>      AVBufferRef *data_ref;
>  
> @@ -130,8 +131,9 @@ typedef struct CodedBitstreamFragment {
>       */
>      size_t data_bit_padding;
>      /**
> -     * If data is reference counted, a reference to the buffer containing
> -     * data.  Null if data is not reference counted.
> +     * A reference to the buffer containing data.
> +     *
> +     * Must be set if data is not NULL.
>       */
>      AVBufferRef *data_ref;

LGTM.
Mark Thompson May 1, 2018, 10:45 p.m.
On 01/05/18 16:51, James Almer wrote:
> On 4/30/2018 8:26 PM, Mark Thompson wrote:
>> Make this clear in the documentation and add some asserts to ensure
>> that it is always true.
>> ---
>> As suggested earlier :)
>>
>>
>>  libavcodec/cbs.c | 18 ++++++++++++------
>>  libavcodec/cbs.h | 10 ++++++----
>>  2 files changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
>> index d81b4e03f7..0d6ffe8824 100644
>> --- a/libavcodec/cbs.c
>> +++ b/libavcodec/cbs.c
>> @@ -140,26 +140,30 @@ static int cbs_read_fragment_content(CodedBitstreamContext *ctx,
>>      int err, i, j;
>>  
>>      for (i = 0; i < frag->nb_units; i++) {
>> +        CodedBitstreamUnit *unit = &frag->units[i];
>> +
>>          if (ctx->decompose_unit_types) {
>>              for (j = 0; j < ctx->nb_decompose_unit_types; j++) {
>> -                if (ctx->decompose_unit_types[j] == frag->units[i].type)
>> +                if (ctx->decompose_unit_types[j] == unit->type)
>>                      break;
>>              }
>>              if (j >= ctx->nb_decompose_unit_types)
>>                  continue;
>>          }
>>  
>> -        av_buffer_unref(&frag->units[i].content_ref);
>> -        frag->units[i].content = NULL;
>> +        av_buffer_unref(&unit->content_ref);
>> +        unit->content = NULL;
>> +
>> +        av_assert0(unit->data && unit->data_ref);
>>  
>> -        err = ctx->codec->read_unit(ctx, &frag->units[i]);
>> +        err = ctx->codec->read_unit(ctx, unit);
>>          if (err == AVERROR(ENOSYS)) {
>>              av_log(ctx->log_ctx, AV_LOG_VERBOSE,
>>                     "Decomposition unimplemented for unit %d "
>> -                   "(type %"PRIu32").\n", i, frag->units[i].type);
>> +                   "(type %"PRIu32").\n", i, unit->type);
>>          } else if (err < 0) {
>>              av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to read unit %d "
>> -                   "(type %"PRIu32").\n", i, frag->units[i].type);
>> +                   "(type %"PRIu32").\n", i, unit->type);
>>              return err;
>>          }
>>      }
>> @@ -277,6 +281,7 @@ int ff_cbs_write_fragment_data(CodedBitstreamContext *ctx,
>>                     "(type %"PRIu32").\n", i, unit->type);
>>              return err;
>>          }
>> +        av_assert0(unit->data && unit->data_ref);
>>      }
>>  
>>      av_buffer_unref(&frag->data_ref);
>> @@ -287,6 +292,7 @@ int ff_cbs_write_fragment_data(CodedBitstreamContext *ctx,
>>          av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to assemble fragment.\n");
>>          return err;
>>      }
>> +    av_assert0(frag->data && frag->data_ref);
> 
> You can remove the assert in ff_cbs_write_packet() if you add this one.

Yes, that would be sensible - removed.

>>  
>>      return 0;
>>  }
>> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
>> index 402eb39e00..487358afaf 100644
>> --- a/libavcodec/cbs.h
>> +++ b/libavcodec/cbs.h
>> @@ -84,8 +84,9 @@ typedef struct CodedBitstreamUnit {
>>       */
>>      size_t   data_bit_padding;
>>      /**
>> -     * If data is reference counted, a reference to the buffer containing
>> -     * data.  Null if data is not reference counted.
>> +     * A reference to the buffer containing data.
>> +     *
>> +     * Must be set if data is not NULL.
>>       */
>>      AVBufferRef *data_ref;
>>  
>> @@ -130,8 +131,9 @@ typedef struct CodedBitstreamFragment {
>>       */
>>      size_t data_bit_padding;
>>      /**
>> -     * If data is reference counted, a reference to the buffer containing
>> -     * data.  Null if data is not reference counted.
>> +     * A reference to the buffer containing data.
>> +     *
>> +     * Must be set if data is not NULL.
>>       */
>>      AVBufferRef *data_ref;
> 
> LGTM.

And applied.

Thanks,

- Mark

Patch hide | download patch | download mbox

diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
index d81b4e03f7..0d6ffe8824 100644
--- a/libavcodec/cbs.c
+++ b/libavcodec/cbs.c
@@ -140,26 +140,30 @@  static int cbs_read_fragment_content(CodedBitstreamContext *ctx,
     int err, i, j;
 
     for (i = 0; i < frag->nb_units; i++) {
+        CodedBitstreamUnit *unit = &frag->units[i];
+
         if (ctx->decompose_unit_types) {
             for (j = 0; j < ctx->nb_decompose_unit_types; j++) {
-                if (ctx->decompose_unit_types[j] == frag->units[i].type)
+                if (ctx->decompose_unit_types[j] == unit->type)
                     break;
             }
             if (j >= ctx->nb_decompose_unit_types)
                 continue;
         }
 
-        av_buffer_unref(&frag->units[i].content_ref);
-        frag->units[i].content = NULL;
+        av_buffer_unref(&unit->content_ref);
+        unit->content = NULL;
+
+        av_assert0(unit->data && unit->data_ref);
 
-        err = ctx->codec->read_unit(ctx, &frag->units[i]);
+        err = ctx->codec->read_unit(ctx, unit);
         if (err == AVERROR(ENOSYS)) {
             av_log(ctx->log_ctx, AV_LOG_VERBOSE,
                    "Decomposition unimplemented for unit %d "
-                   "(type %"PRIu32").\n", i, frag->units[i].type);
+                   "(type %"PRIu32").\n", i, unit->type);
         } else if (err < 0) {
             av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to read unit %d "
-                   "(type %"PRIu32").\n", i, frag->units[i].type);
+                   "(type %"PRIu32").\n", i, unit->type);
             return err;
         }
     }
@@ -277,6 +281,7 @@  int ff_cbs_write_fragment_data(CodedBitstreamContext *ctx,
                    "(type %"PRIu32").\n", i, unit->type);
             return err;
         }
+        av_assert0(unit->data && unit->data_ref);
     }
 
     av_buffer_unref(&frag->data_ref);
@@ -287,6 +292,7 @@  int ff_cbs_write_fragment_data(CodedBitstreamContext *ctx,
         av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to assemble fragment.\n");
         return err;
     }
+    av_assert0(frag->data && frag->data_ref);
 
     return 0;
 }
diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
index 402eb39e00..487358afaf 100644
--- a/libavcodec/cbs.h
+++ b/libavcodec/cbs.h
@@ -84,8 +84,9 @@  typedef struct CodedBitstreamUnit {
      */
     size_t   data_bit_padding;
     /**
-     * If data is reference counted, a reference to the buffer containing
-     * data.  Null if data is not reference counted.
+     * A reference to the buffer containing data.
+     *
+     * Must be set if data is not NULL.
      */
     AVBufferRef *data_ref;
 
@@ -130,8 +131,9 @@  typedef struct CodedBitstreamFragment {
      */
     size_t data_bit_padding;
     /**
-     * If data is reference counted, a reference to the buffer containing
-     * data.  Null if data is not reference counted.
+     * A reference to the buffer containing data.
+     *
+     * Must be set if data is not NULL.
      */
     AVBufferRef *data_ref;