| Message ID | 20180430232605.4846-1-sw@jkqxz.net |
|---|---|
| State | New |
| Headers | show |
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.
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
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;