Message ID | 20190617034223.21195-11-andreas.rheinhardt@gmail.com |
---|---|
State | Superseded |
Headers | show |
On 6/17/2019 12:42 AM, Andreas Rheinhardt wrote: > ff_cbs_delete_unit never fails if the index of the unit to delete is > valid; document this behaviour explicitly and remove the checks for > whether ff_cbs_delete_unit failed, because all the callers of > ff_cbs_delete_unit already make sure that the index is valid. Add a comment about why you're ignoring the return values in all three filters. If for whatever reason the filters are changed in the future and it can no longer be ensured the call will never fail, the developer should be aware of it. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > Several callers already ignored to check the return value. > > libavcodec/av1_metadata_bsf.c | 9 ++------- > libavcodec/cbs.h | 2 ++ > libavcodec/h264_metadata_bsf.c | 7 +------ > libavcodec/h264_redundant_pps_bsf.c | 4 +--- > 4 files changed, 6 insertions(+), 16 deletions(-) > > diff --git a/libavcodec/av1_metadata_bsf.c b/libavcodec/av1_metadata_bsf.c > index 842b80c201..dafddced63 100644 > --- a/libavcodec/av1_metadata_bsf.c > +++ b/libavcodec/av1_metadata_bsf.c > @@ -161,13 +161,8 @@ static int av1_metadata_filter(AVBSFContext *bsf, AVPacket *pkt) > > if (ctx->delete_padding) { > for (i = frag->nb_units - 1; i >= 0; i--) { > - if (frag->units[i].type == AV1_OBU_PADDING) { > - err = ff_cbs_delete_unit(ctx->cbc, frag, i); > - if (err < 0) { > - av_log(bsf, AV_LOG_ERROR, "Failed to delete Padding OBU.\n"); > - goto fail; > - } > - } > + if (frag->units[i].type == AV1_OBU_PADDING) > + ff_cbs_delete_unit(ctx->cbc, frag, i); > } > } > > diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h > index 5260a39c63..e1e6055ceb 100644 > --- a/libavcodec/cbs.h > +++ b/libavcodec/cbs.h > @@ -380,6 +380,8 @@ int ff_cbs_insert_unit_data(CodedBitstreamContext *ctx, > > /** > * Delete a unit from a fragment and free all memory it uses. > + * > + * Never returns failure if position is >= 0 and < frag->nb_units. > */ > int ff_cbs_delete_unit(CodedBitstreamContext *ctx, > CodedBitstreamFragment *frag, > diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c > index f7ca1f0f09..d05b75be14 100644 > --- a/libavcodec/h264_metadata_bsf.c > +++ b/libavcodec/h264_metadata_bsf.c > @@ -428,12 +428,7 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt) > for (i = au->nb_units - 1; i >= 0; i--) { > if (au->units[i].type == H264_NAL_FILLER_DATA) { > // Filler NAL units. > - err = ff_cbs_delete_unit(ctx->cbc, au, i); > - if (err < 0) { > - av_log(bsf, AV_LOG_ERROR, "Failed to delete " > - "filler NAL.\n"); > - goto fail; > - } > + ff_cbs_delete_unit(ctx->cbc, au, i); > continue; > } > > diff --git a/libavcodec/h264_redundant_pps_bsf.c b/libavcodec/h264_redundant_pps_bsf.c > index db8717d69a..8526b5b4c4 100644 > --- a/libavcodec/h264_redundant_pps_bsf.c > +++ b/libavcodec/h264_redundant_pps_bsf.c > @@ -95,9 +95,7 @@ static int h264_redundant_pps_filter(AVBSFContext *bsf, AVPacket *out) > if (!au_has_sps) { > av_log(bsf, AV_LOG_VERBOSE, "Deleting redundant PPS " > "at %"PRId64".\n", in->pts); > - err = ff_cbs_delete_unit(ctx->input, au, i); > - if (err < 0) > - goto fail; > + ff_cbs_delete_unit(ctx->input, au, i); > } > } > if (nal->type == H264_NAL_SLICE || >
James Almer: > On 6/17/2019 12:42 AM, Andreas Rheinhardt wrote: >> ff_cbs_delete_unit never fails if the index of the unit to delete is >> valid; document this behaviour explicitly and remove the checks for >> whether ff_cbs_delete_unit failed, because all the callers of >> ff_cbs_delete_unit already make sure that the index is valid. > > Add a comment about why you're ignoring the return values in all three > filters. If for whatever reason the filters are changed in the future > and it can no longer be ensured the call will never fail, the developer > should be aware of it. > Ok. I just noticed that the call for deleting AV1 temporal delimiters doesn't check whether the fragment has a unit at all (in fact, the check before that call already presupposes it). The only way for ff_cbs_read_packet to return an empty fragment and no error is if the packet's buf has a size of zero (this is legal according to the API if the packet has side-data). What should be done about packets that don't result in a single unit and more specifically about zero-sized packets? The obvious answer would be to simply add a check to av1_metadata like the already existing in h264_metadata; but discarding side-data only packets is no good. (If the zero-sized packet had new extradata as side-data, then that's something that may be parsed and rewritten; the other side-data stuff should simply be passed through.) - Andreas
diff --git a/libavcodec/av1_metadata_bsf.c b/libavcodec/av1_metadata_bsf.c index 842b80c201..dafddced63 100644 --- a/libavcodec/av1_metadata_bsf.c +++ b/libavcodec/av1_metadata_bsf.c @@ -161,13 +161,8 @@ static int av1_metadata_filter(AVBSFContext *bsf, AVPacket *pkt) if (ctx->delete_padding) { for (i = frag->nb_units - 1; i >= 0; i--) { - if (frag->units[i].type == AV1_OBU_PADDING) { - err = ff_cbs_delete_unit(ctx->cbc, frag, i); - if (err < 0) { - av_log(bsf, AV_LOG_ERROR, "Failed to delete Padding OBU.\n"); - goto fail; - } - } + if (frag->units[i].type == AV1_OBU_PADDING) + ff_cbs_delete_unit(ctx->cbc, frag, i); } } diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h index 5260a39c63..e1e6055ceb 100644 --- a/libavcodec/cbs.h +++ b/libavcodec/cbs.h @@ -380,6 +380,8 @@ int ff_cbs_insert_unit_data(CodedBitstreamContext *ctx, /** * Delete a unit from a fragment and free all memory it uses. + * + * Never returns failure if position is >= 0 and < frag->nb_units. */ int ff_cbs_delete_unit(CodedBitstreamContext *ctx, CodedBitstreamFragment *frag, diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c index f7ca1f0f09..d05b75be14 100644 --- a/libavcodec/h264_metadata_bsf.c +++ b/libavcodec/h264_metadata_bsf.c @@ -428,12 +428,7 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt) for (i = au->nb_units - 1; i >= 0; i--) { if (au->units[i].type == H264_NAL_FILLER_DATA) { // Filler NAL units. - err = ff_cbs_delete_unit(ctx->cbc, au, i); - if (err < 0) { - av_log(bsf, AV_LOG_ERROR, "Failed to delete " - "filler NAL.\n"); - goto fail; - } + ff_cbs_delete_unit(ctx->cbc, au, i); continue; } diff --git a/libavcodec/h264_redundant_pps_bsf.c b/libavcodec/h264_redundant_pps_bsf.c index db8717d69a..8526b5b4c4 100644 --- a/libavcodec/h264_redundant_pps_bsf.c +++ b/libavcodec/h264_redundant_pps_bsf.c @@ -95,9 +95,7 @@ static int h264_redundant_pps_filter(AVBSFContext *bsf, AVPacket *out) if (!au_has_sps) { av_log(bsf, AV_LOG_VERBOSE, "Deleting redundant PPS " "at %"PRId64".\n", in->pts); - err = ff_cbs_delete_unit(ctx->input, au, i); - if (err < 0) - goto fail; + ff_cbs_delete_unit(ctx->input, au, i); } } if (nal->type == H264_NAL_SLICE ||
ff_cbs_delete_unit never fails if the index of the unit to delete is valid; document this behaviour explicitly and remove the checks for whether ff_cbs_delete_unit failed, because all the callers of ff_cbs_delete_unit already make sure that the index is valid. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- Several callers already ignored to check the return value. libavcodec/av1_metadata_bsf.c | 9 ++------- libavcodec/cbs.h | 2 ++ libavcodec/h264_metadata_bsf.c | 7 +------ libavcodec/h264_redundant_pps_bsf.c | 4 +--- 4 files changed, 6 insertions(+), 16 deletions(-)