| Message ID | 20190205200852.2160-3-andreas.rheinhardt@googlemail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 05/02/2019 20:08, Andreas Rheinhardt wrote: > Up until now, a fragment that got reused was zeroed twice: Once during > uninit and once during reading the next packet/extradata/buffer. The > second zeroing has now been made optional. > > This is also in preparation of actually reusing a fragment's units array. > Otherwise it would be lost during reading. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@googlemail.com> > --- > libavcodec/av1_metadata_bsf.c | 4 ++-- > libavcodec/av1_parser.c | 4 ++-- > libavcodec/cbs.c | 17 +++++++++++------ > libavcodec/cbs.h | 20 +++++++++++++++++--- > libavcodec/filter_units_bsf.c | 4 ++-- > libavcodec/h264_metadata_bsf.c | 4 ++-- > libavcodec/h264_redundant_pps_bsf.c | 4 ++-- > libavcodec/h265_metadata_bsf.c | 4 ++-- > libavcodec/mpeg2_metadata_bsf.c | 4 ++-- > libavcodec/trace_headers_bsf.c | 4 ++-- > libavcodec/vp9_metadata_bsf.c | 2 +- > 11 files changed, 45 insertions(+), 26 deletions(-) > > ... > diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c > index b61dedb1eb..71f9fcbe32 100644 > --- a/libavcodec/cbs.c > +++ b/libavcodec/cbs.c > @@ -217,11 +217,13 @@ static int cbs_fill_fragment_data(CodedBitstreamContext *ctx, > > int ff_cbs_read_extradata(CodedBitstreamContext *ctx, > CodedBitstreamFragment *frag, > - const AVCodecParameters *par) > + const AVCodecParameters *par, > + int reuse) > { > int err; > > - memset(frag, 0, sizeof(*frag)); > + if (!reuse) > + memset(frag, 0, sizeof(*frag)); > > err = cbs_fill_fragment_data(ctx, frag, par->extradata, > par->extradata_size); > @@ -237,11 +239,12 @@ int ff_cbs_read_extradata(CodedBitstreamContext *ctx, > > int ff_cbs_read_packet(CodedBitstreamContext *ctx, > CodedBitstreamFragment *frag, > - const AVPacket *pkt) > + const AVPacket *pkt, int reuse) > { > int err; > > - memset(frag, 0, sizeof(*frag)); > + if (!reuse) > + memset(frag, 0, sizeof(*frag)); > > if (pkt->buf) { > frag->data_ref = av_buffer_ref(pkt->buf); > @@ -266,11 +269,13 @@ int ff_cbs_read_packet(CodedBitstreamContext *ctx, > > int ff_cbs_read(CodedBitstreamContext *ctx, > CodedBitstreamFragment *frag, > - const uint8_t *data, size_t size) > + const uint8_t *data, size_t size, > + int reuse) > { > int err; > > - memset(frag, 0, sizeof(*frag)); > + if (!reuse) > + memset(frag, 0, sizeof(*frag)); > > err = cbs_fill_fragment_data(ctx, frag, data, size); > if (err < 0) > diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h > index 229cb129aa..2265b5d5bd 100644 > --- a/libavcodec/cbs.h > +++ b/libavcodec/cbs.h > @@ -240,10 +240,15 @@ void ff_cbs_close(CodedBitstreamContext **ctx); > * This also updates the internal state, so will need to be called for > * codecs with extradata to read parameter sets necessary for further > * parsing even if the fragment itself is not desired. > + * > + * If reuse is not set, the fragment will be zeroed before usage; > + * otherwise, the fragment's units_allocated and units members must > + * be valid and all the other members have to be zero/NULL. > */ > int ff_cbs_read_extradata(CodedBitstreamContext *ctx, > CodedBitstreamFragment *frag, > - const AVCodecParameters *par); > + const AVCodecParameters *par, > + int reuse); > > /** > * Read the data bitstream from a packet into a fragment, then > @@ -252,10 +257,14 @@ int ff_cbs_read_extradata(CodedBitstreamContext *ctx, > * This also updates the internal state of the coded bitstream context > * with any persistent data from the fragment which may be required to > * read following fragments (e.g. parameter sets). > + * > + * If reuse is not set, the fragment will be zeroed before usage; > + * otherwise, the fragment's units_allocated and units members must > + * be valid and all the other members have to be zero/NULL. > */ > int ff_cbs_read_packet(CodedBitstreamContext *ctx, > CodedBitstreamFragment *frag, > - const AVPacket *pkt); > + const AVPacket *pkt, int reuse); > > /** > * Read a bitstream from a memory region into a fragment, then > @@ -264,10 +273,15 @@ int ff_cbs_read_packet(CodedBitstreamContext *ctx, > * This also updates the internal state of the coded bitstream context > * with any persistent data from the fragment which may be required to > * read following fragments (e.g. parameter sets). > + * > + * If reuse is not set, the fragment will be zeroed before usage; > + * otherwise, the fragment's units_allocated and units members must > + * be valid and all the other members have to be zero/NULL. > */ > int ff_cbs_read(CodedBitstreamContext *ctx, > CodedBitstreamFragment *frag, > - const uint8_t *data, size_t size); > + const uint8_t *data, size_t size, > + int reuse); > > > /** I don't think this patch should be needed. Can we just document that your fragment must either be zeroed (so, allocated by av_*allocz() or memset() to zero) or you've called the ff_cbs_fragment_reset() (whatever the name is) function before any read call? It can even check that the user hasn't messed up by asserting that data, data_size and nb_units are all zero. Thanks, - Mark
Mark Thompson: > On 05/02/2019 20:08, Andreas Rheinhardt wrote: >> int ff_cbs_read_extradata(CodedBitstreamContext *ctx, >> CodedBitstreamFragment *frag, >> - const AVCodecParameters *par) >> + const AVCodecParameters *par, >> + int reuse) >> { >> int err; >> >> - memset(frag, 0, sizeof(*frag)); >> + if (!reuse) >> + memset(frag, 0, sizeof(*frag)); >> >> err = cbs_fill_fragment_data(ctx, frag, par->extradata, >> par->extradata_size); >> @@ -237,11 +239,12 @@ int ff_cbs_read_extradata(CodedBitstreamContext *ctx, >> >> int ff_cbs_read_packet(CodedBitstreamContext *ctx, >> CodedBitstreamFragment *frag, >> - const AVPacket *pkt) >> + const AVPacket *pkt, int reuse) >> { >> int err; >> >> - memset(frag, 0, sizeof(*frag)); >> + if (!reuse) >> + memset(frag, 0, sizeof(*frag)); >> >> if (pkt->buf) { >> frag->data_ref = av_buffer_ref(pkt->buf); >> @@ -266,11 +269,13 @@ int ff_cbs_read_packet(CodedBitstreamContext *ctx, >> >> int ff_cbs_read(CodedBitstreamContext *ctx, >> CodedBitstreamFragment *frag, >> - const uint8_t *data, size_t size) >> + const uint8_t *data, size_t size, >> + int reuse) >> { >> int err; >> >> - memset(frag, 0, sizeof(*frag)); >> + if (!reuse) >> + memset(frag, 0, sizeof(*frag)); >> >> err = cbs_fill_fragment_data(ctx, frag, data, size); >> if (err < 0) > > I don't think this patch should be needed. Can we just document that your fragment must either be zeroed (so, allocated by av_*allocz() or memset() to zero) or you've called the ff_cbs_fragment_reset() (whatever the name is) function before any read call? It can even check that the user hasn't messed up by asserting that data, data_size and nb_units are all zero. I agree with your suggestion regarding the documentation; but it is nevertheless needed to eliminate the memset in the ff_cbs_read functions. Otherwise the unit array is leaked. Andreas
On 10/02/2019 23:11, Andreas Rheinhardt wrote: > Mark Thompson: >> On 05/02/2019 20:08, Andreas Rheinhardt wrote: >>> int ff_cbs_read_extradata(CodedBitstreamContext *ctx, >>> CodedBitstreamFragment *frag, >>> - const AVCodecParameters *par) >>> + const AVCodecParameters *par, >>> + int reuse) >>> { >>> int err; >>> >>> - memset(frag, 0, sizeof(*frag)); >>> + if (!reuse) >>> + memset(frag, 0, sizeof(*frag)); >>> >>> err = cbs_fill_fragment_data(ctx, frag, par->extradata, >>> par->extradata_size); >>> @@ -237,11 +239,12 @@ int ff_cbs_read_extradata(CodedBitstreamContext *ctx, >>> >>> int ff_cbs_read_packet(CodedBitstreamContext *ctx, >>> CodedBitstreamFragment *frag, >>> - const AVPacket *pkt) >>> + const AVPacket *pkt, int reuse) >>> { >>> int err; >>> >>> - memset(frag, 0, sizeof(*frag)); >>> + if (!reuse) >>> + memset(frag, 0, sizeof(*frag)); >>> >>> if (pkt->buf) { >>> frag->data_ref = av_buffer_ref(pkt->buf); >>> @@ -266,11 +269,13 @@ int ff_cbs_read_packet(CodedBitstreamContext *ctx, >>> >>> int ff_cbs_read(CodedBitstreamContext *ctx, >>> CodedBitstreamFragment *frag, >>> - const uint8_t *data, size_t size) >>> + const uint8_t *data, size_t size, >>> + int reuse) >>> { >>> int err; >>> >>> - memset(frag, 0, sizeof(*frag)); >>> + if (!reuse) >>> + memset(frag, 0, sizeof(*frag)); >>> >>> err = cbs_fill_fragment_data(ctx, frag, data, size); >>> if (err < 0) >> >> I don't think this patch should be needed. Can we just document that your fragment must either be zeroed (so, allocated by av_*allocz() or memset() to zero) or you've called the ff_cbs_fragment_reset() (whatever the name is) function before any read call? It can even check that the user hasn't messed up by asserting that data, data_size and nb_units are all zero. > > I agree with your suggestion regarding the documentation; but it is > nevertheless needed to eliminate the memset in the ff_cbs_read > functions. Otherwise the unit array is leaked. Yes, sorry. Indeed still remove the memset, but don't change any of the parameters or callers. Thanks, - Mark
diff --git a/libavcodec/av1_metadata_bsf.c b/libavcodec/av1_metadata_bsf.c index c3c56afeab..b08a1379e7 100644 --- a/libavcodec/av1_metadata_bsf.c +++ b/libavcodec/av1_metadata_bsf.c @@ -126,7 +126,7 @@ static int av1_metadata_filter(AVBSFContext *bsf, AVPacket *out) if (err < 0) return err; - err = ff_cbs_read_packet(ctx->cbc, frag, in); + err = ff_cbs_read_packet(ctx->cbc, frag, in, 1); if (err < 0) { av_log(bsf, AV_LOG_ERROR, "Failed to read packet.\n"); goto fail; @@ -191,7 +191,7 @@ static int av1_metadata_init(AVBSFContext *bsf) return err; if (bsf->par_in->extradata) { - err = ff_cbs_read_extradata(ctx->cbc, frag, bsf->par_in); + err = ff_cbs_read_extradata(ctx->cbc, frag, bsf->par_in, 1); if (err < 0) { av_log(bsf, AV_LOG_ERROR, "Failed to read extradata.\n"); goto fail; diff --git a/libavcodec/av1_parser.c b/libavcodec/av1_parser.c index d78e4b3f3a..071227eed6 100644 --- a/libavcodec/av1_parser.c +++ b/libavcodec/av1_parser.c @@ -66,7 +66,7 @@ static int av1_parser_parse(AVCodecParserContext *ctx, if (avctx->extradata_size && !s->parsed_extradata) { s->parsed_extradata = 1; - ret = ff_cbs_read(s->cbc, td, avctx->extradata, avctx->extradata_size); + ret = ff_cbs_read(s->cbc, td, avctx->extradata, avctx->extradata_size, 1); if (ret < 0) { av_log(avctx, AV_LOG_ERROR, "Failed to parse extradata.\n"); goto end; @@ -75,7 +75,7 @@ static int av1_parser_parse(AVCodecParserContext *ctx, ff_cbs_fragment_uninit(s->cbc, td, 1); } - ret = ff_cbs_read(s->cbc, td, data, size); + ret = ff_cbs_read(s->cbc, td, data, size, 1); if (ret < 0) { av_log(avctx, AV_LOG_ERROR, "Failed to parse temporal unit.\n"); goto end; diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c index b61dedb1eb..71f9fcbe32 100644 --- a/libavcodec/cbs.c +++ b/libavcodec/cbs.c @@ -217,11 +217,13 @@ static int cbs_fill_fragment_data(CodedBitstreamContext *ctx, int ff_cbs_read_extradata(CodedBitstreamContext *ctx, CodedBitstreamFragment *frag, - const AVCodecParameters *par) + const AVCodecParameters *par, + int reuse) { int err; - memset(frag, 0, sizeof(*frag)); + if (!reuse) + memset(frag, 0, sizeof(*frag)); err = cbs_fill_fragment_data(ctx, frag, par->extradata, par->extradata_size); @@ -237,11 +239,12 @@ int ff_cbs_read_extradata(CodedBitstreamContext *ctx, int ff_cbs_read_packet(CodedBitstreamContext *ctx, CodedBitstreamFragment *frag, - const AVPacket *pkt) + const AVPacket *pkt, int reuse) { int err; - memset(frag, 0, sizeof(*frag)); + if (!reuse) + memset(frag, 0, sizeof(*frag)); if (pkt->buf) { frag->data_ref = av_buffer_ref(pkt->buf); @@ -266,11 +269,13 @@ int ff_cbs_read_packet(CodedBitstreamContext *ctx, int ff_cbs_read(CodedBitstreamContext *ctx, CodedBitstreamFragment *frag, - const uint8_t *data, size_t size) + const uint8_t *data, size_t size, + int reuse) { int err; - memset(frag, 0, sizeof(*frag)); + if (!reuse) + memset(frag, 0, sizeof(*frag)); err = cbs_fill_fragment_data(ctx, frag, data, size); if (err < 0) diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h index 229cb129aa..2265b5d5bd 100644 --- a/libavcodec/cbs.h +++ b/libavcodec/cbs.h @@ -240,10 +240,15 @@ void ff_cbs_close(CodedBitstreamContext **ctx); * This also updates the internal state, so will need to be called for * codecs with extradata to read parameter sets necessary for further * parsing even if the fragment itself is not desired. + * + * If reuse is not set, the fragment will be zeroed before usage; + * otherwise, the fragment's units_allocated and units members must + * be valid and all the other members have to be zero/NULL. */ int ff_cbs_read_extradata(CodedBitstreamContext *ctx, CodedBitstreamFragment *frag, - const AVCodecParameters *par); + const AVCodecParameters *par, + int reuse); /** * Read the data bitstream from a packet into a fragment, then @@ -252,10 +257,14 @@ int ff_cbs_read_extradata(CodedBitstreamContext *ctx, * This also updates the internal state of the coded bitstream context * with any persistent data from the fragment which may be required to * read following fragments (e.g. parameter sets). + * + * If reuse is not set, the fragment will be zeroed before usage; + * otherwise, the fragment's units_allocated and units members must + * be valid and all the other members have to be zero/NULL. */ int ff_cbs_read_packet(CodedBitstreamContext *ctx, CodedBitstreamFragment *frag, - const AVPacket *pkt); + const AVPacket *pkt, int reuse); /** * Read a bitstream from a memory region into a fragment, then @@ -264,10 +273,15 @@ int ff_cbs_read_packet(CodedBitstreamContext *ctx, * This also updates the internal state of the coded bitstream context * with any persistent data from the fragment which may be required to * read following fragments (e.g. parameter sets). + * + * If reuse is not set, the fragment will be zeroed before usage; + * otherwise, the fragment's units_allocated and units members must + * be valid and all the other members have to be zero/NULL. */ int ff_cbs_read(CodedBitstreamContext *ctx, CodedBitstreamFragment *frag, - const uint8_t *data, size_t size); + const uint8_t *data, size_t size, + int reuse); /** diff --git a/libavcodec/filter_units_bsf.c b/libavcodec/filter_units_bsf.c index a3b25cb944..9980601ce5 100644 --- a/libavcodec/filter_units_bsf.c +++ b/libavcodec/filter_units_bsf.c @@ -116,7 +116,7 @@ static int filter_units_filter(AVBSFContext *bsf, AVPacket *out) return 0; } - err = ff_cbs_read_packet(ctx->cbc, frag, in); + err = ff_cbs_read_packet(ctx->cbc, frag, in, 1); if (err < 0) { av_log(bsf, AV_LOG_ERROR, "Failed to read packet.\n"); goto fail; @@ -201,7 +201,7 @@ static int filter_units_init(AVBSFContext *bsf) if (bsf->par_in->extradata) { CodedBitstreamFragment ps; - err = ff_cbs_read_extradata(ctx->cbc, &ps, bsf->par_in); + err = ff_cbs_read_extradata(ctx->cbc, &ps, bsf->par_in, 0); if (err < 0) { av_log(bsf, AV_LOG_ERROR, "Failed to read extradata.\n"); } else { diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c index c4cfc6094f..53ba623b70 100644 --- a/libavcodec/h264_metadata_bsf.c +++ b/libavcodec/h264_metadata_bsf.c @@ -291,7 +291,7 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *out) if (err < 0) return err; - err = ff_cbs_read_packet(ctx->cbc, au, in); + err = ff_cbs_read_packet(ctx->cbc, au, in, 1); if (err < 0) { av_log(bsf, AV_LOG_ERROR, "Failed to read packet.\n"); goto fail; @@ -625,7 +625,7 @@ static int h264_metadata_init(AVBSFContext *bsf) return err; if (bsf->par_in->extradata) { - err = ff_cbs_read_extradata(ctx->cbc, au, bsf->par_in); + err = ff_cbs_read_extradata(ctx->cbc, au, bsf->par_in, 1); if (err < 0) { av_log(bsf, AV_LOG_ERROR, "Failed to read extradata.\n"); goto fail; diff --git a/libavcodec/h264_redundant_pps_bsf.c b/libavcodec/h264_redundant_pps_bsf.c index 1c929cd44b..1df93dd121 100644 --- a/libavcodec/h264_redundant_pps_bsf.c +++ b/libavcodec/h264_redundant_pps_bsf.c @@ -78,7 +78,7 @@ static int h264_redundant_pps_filter(AVBSFContext *bsf, AVPacket *out) if (err < 0) return err; - err = ff_cbs_read_packet(ctx->input, au, in); + err = ff_cbs_read_packet(ctx->input, au, in, 1); if (err < 0) goto fail; @@ -143,7 +143,7 @@ static int h264_redundant_pps_init(AVBSFContext *bsf) ctx->global_pic_init_qp = 26; if (bsf->par_in->extradata) { - err = ff_cbs_read_extradata(ctx->input, au, bsf->par_in); + err = ff_cbs_read_extradata(ctx->input, au, bsf->par_in, 1); if (err < 0) { av_log(bsf, AV_LOG_ERROR, "Failed to read extradata.\n"); goto fail; diff --git a/libavcodec/h265_metadata_bsf.c b/libavcodec/h265_metadata_bsf.c index 0ad2ea80b8..53d35f0e09 100644 --- a/libavcodec/h265_metadata_bsf.c +++ b/libavcodec/h265_metadata_bsf.c @@ -241,7 +241,7 @@ static int h265_metadata_filter(AVBSFContext *bsf, AVPacket *out) if (err < 0) return err; - err = ff_cbs_read_packet(ctx->cbc, au, in); + err = ff_cbs_read_packet(ctx->cbc, au, in, 1); if (err < 0) { av_log(bsf, AV_LOG_ERROR, "Failed to read packet.\n"); goto fail; @@ -342,7 +342,7 @@ static int h265_metadata_init(AVBSFContext *bsf) return err; if (bsf->par_in->extradata) { - err = ff_cbs_read_extradata(ctx->cbc, au, bsf->par_in); + err = ff_cbs_read_extradata(ctx->cbc, au, bsf->par_in, 1); if (err < 0) { av_log(bsf, AV_LOG_ERROR, "Failed to read extradata.\n"); goto fail; diff --git a/libavcodec/mpeg2_metadata_bsf.c b/libavcodec/mpeg2_metadata_bsf.c index 420b620f6a..43e6ac8c15 100644 --- a/libavcodec/mpeg2_metadata_bsf.c +++ b/libavcodec/mpeg2_metadata_bsf.c @@ -190,7 +190,7 @@ static int mpeg2_metadata_filter(AVBSFContext *bsf, AVPacket *out) if (err < 0) return err; - err = ff_cbs_read_packet(ctx->cbc, frag, in); + err = ff_cbs_read_packet(ctx->cbc, frag, in, 1); if (err < 0) { av_log(bsf, AV_LOG_ERROR, "Failed to read packet.\n"); goto fail; @@ -234,7 +234,7 @@ static int mpeg2_metadata_init(AVBSFContext *bsf) return err; if (bsf->par_in->extradata) { - err = ff_cbs_read_extradata(ctx->cbc, frag, bsf->par_in); + err = ff_cbs_read_extradata(ctx->cbc, frag, bsf->par_in, 1); if (err < 0) { av_log(bsf, AV_LOG_ERROR, "Failed to read extradata.\n"); goto fail; diff --git a/libavcodec/trace_headers_bsf.c b/libavcodec/trace_headers_bsf.c index f9667f0456..c6a47e6d6d 100644 --- a/libavcodec/trace_headers_bsf.c +++ b/libavcodec/trace_headers_bsf.c @@ -48,7 +48,7 @@ static int trace_headers_init(AVBSFContext *bsf) av_log(bsf, AV_LOG_INFO, "Extradata\n"); - err = ff_cbs_read_extradata(ctx->cbc, &ps, bsf->par_in); + err = ff_cbs_read_extradata(ctx->cbc, &ps, bsf->par_in, 0); ff_cbs_fragment_uninit(ctx->cbc, &ps, 1); } @@ -92,7 +92,7 @@ static int trace_headers(AVBSFContext *bsf, AVPacket *pkt) av_log(bsf, AV_LOG_INFO, "Packet: %d bytes%s.\n", pkt->size, tmp); - err = ff_cbs_read_packet(ctx->cbc, &au, pkt); + err = ff_cbs_read_packet(ctx->cbc, &au, pkt, 0); ff_cbs_fragment_uninit(ctx->cbc, &au, 1); diff --git a/libavcodec/vp9_metadata_bsf.c b/libavcodec/vp9_metadata_bsf.c index b275a07800..26f1e65f20 100644 --- a/libavcodec/vp9_metadata_bsf.c +++ b/libavcodec/vp9_metadata_bsf.c @@ -48,7 +48,7 @@ static int vp9_metadata_filter(AVBSFContext *bsf, AVPacket *out) if (err < 0) return err; - err = ff_cbs_read_packet(ctx->cbc, frag, in); + err = ff_cbs_read_packet(ctx->cbc, frag, in, 1); if (err < 0) { av_log(bsf, AV_LOG_ERROR, "Failed to read packet.\n"); goto fail;
Up until now, a fragment that got reused was zeroed twice: Once during uninit and once during reading the next packet/extradata/buffer. The second zeroing has now been made optional. This is also in preparation of actually reusing a fragment's units array. Otherwise it would be lost during reading. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@googlemail.com> --- libavcodec/av1_metadata_bsf.c | 4 ++-- libavcodec/av1_parser.c | 4 ++-- libavcodec/cbs.c | 17 +++++++++++------ libavcodec/cbs.h | 20 +++++++++++++++++--- libavcodec/filter_units_bsf.c | 4 ++-- libavcodec/h264_metadata_bsf.c | 4 ++-- libavcodec/h264_redundant_pps_bsf.c | 4 ++-- libavcodec/h265_metadata_bsf.c | 4 ++-- libavcodec/mpeg2_metadata_bsf.c | 4 ++-- libavcodec/trace_headers_bsf.c | 4 ++-- libavcodec/vp9_metadata_bsf.c | 2 +- 11 files changed, 45 insertions(+), 26 deletions(-)