| Message ID | 20190710210827.20409-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | eb17bf6fd3da5136d3b15c7e608a317826fd15f9 |
| Headers | show |
Andreas Rheinhardt: > 1. Since bd90a2ec, mpeg4_unpack_bframes caches whole packets instead of > just the pointer to the buffer and the buffer's size in order to be able > to make use of refcounting to avoid copying of data; this unfortunately > introduced copies of packet structures and side data (if existing), > although the only fields that are needed are the buffer-related ones > (data, size and buf). This can be changed without compromising the > advantages of refcounting by storing a reference to the buffer. > > 2. This change also makes it easy to use only one packet throughout > so that an allocation and free of an AVPacket structure per filtered > packet can be saved by switching to ff_bsf_get_packet_ref. > > 3. Furthermore, this commit also fixes a memleak introduced in bd90a2ec: > If a stored b_frame with side data was used for a later frame, the side > data would leak when the input frame's properties were copied into the > output frame. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavcodec/mpeg4_unpack_bframes_bsf.c | 77 ++++++++++++--------------- > 1 file changed, 35 insertions(+), 42 deletions(-) > > diff --git a/libavcodec/mpeg4_unpack_bframes_bsf.c b/libavcodec/mpeg4_unpack_bframes_bsf.c > index 1daf133ce5..382070b423 100644 > --- a/libavcodec/mpeg4_unpack_bframes_bsf.c > +++ b/libavcodec/mpeg4_unpack_bframes_bsf.c > @@ -25,7 +25,7 @@ > #include "mpeg4video.h" > > typedef struct UnpackBFramesBSFContext { > - AVPacket *b_frame; > + AVBufferRef *b_frame_ref; > } UnpackBFramesBSFContext; > > /* determine the position of the packed marker in the userdata, > @@ -56,32 +56,32 @@ static void scan_buffer(const uint8_t *buf, int buf_size, > } > } > > -static int mpeg4_unpack_bframes_filter(AVBSFContext *ctx, AVPacket *out) > +static int mpeg4_unpack_bframes_filter(AVBSFContext *ctx, AVPacket *pkt) > { > UnpackBFramesBSFContext *s = ctx->priv_data; > int pos_p = -1, nb_vop = 0, pos_vop2 = -1, ret = 0; > - AVPacket *in; > > - ret = ff_bsf_get_packet(ctx, &in); > + ret = ff_bsf_get_packet_ref(ctx, pkt); > if (ret < 0) > return ret; > > - scan_buffer(in->data, in->size, &pos_p, &nb_vop, &pos_vop2); > + scan_buffer(pkt->data, pkt->size, &pos_p, &nb_vop, &pos_vop2); > av_log(ctx, AV_LOG_DEBUG, "Found %d VOP startcode(s) in this packet.\n", nb_vop); > > if (pos_vop2 >= 0) { > - if (s->b_frame->data) { > + if (s->b_frame_ref) { > av_log(ctx, AV_LOG_WARNING, > "Missing one N-VOP packet, discarding one B-frame.\n"); > - av_packet_unref(s->b_frame); > + av_buffer_unref(&s->b_frame_ref); > } > - /* store the packed B-frame in the BSFContext */ > - ret = av_packet_ref(s->b_frame, in); > - if (ret < 0) { > + /* store a reference to the packed B-frame's data in the BSFContext */ > + s->b_frame_ref = av_buffer_ref(pkt->buf); > + if (!s->b_frame_ref) { > + ret = AVERROR(ENOMEM); > goto fail; > } > - s->b_frame->size -= pos_vop2; > - s->b_frame->data += pos_vop2; > + s->b_frame_ref->data = pkt->data + pos_vop2; > + s->b_frame_ref->size = pkt->size - pos_vop2; > } > > if (nb_vop > 2) { > @@ -89,56 +89,49 @@ static int mpeg4_unpack_bframes_filter(AVBSFContext *ctx, AVPacket *out) > "Found %d VOP headers in one packet, only unpacking one.\n", nb_vop); > } > > - if (nb_vop == 1 && s->b_frame->data) { > - /* use frame from BSFContext */ > - av_packet_move_ref(out, s->b_frame); > + if (nb_vop == 1 && s->b_frame_ref) { > + AVBufferRef *tmp = pkt->buf; > > - /* use properties from current input packet */ > - ret = av_packet_copy_props(out, in); > - if (ret < 0) { > - goto fail; > - } > + /* make tmp accurately reflect the packet's data */ > + tmp->data = pkt->data; > + tmp->size = pkt->size; > + > + /* replace data in packet with stored data */ > + pkt->buf = s->b_frame_ref; > + pkt->data = s->b_frame_ref->data; > + pkt->size = s->b_frame_ref->size; > + > + /* store reference to data into BSFContext */ > + s->b_frame_ref = tmp; > > - if (in->size <= MAX_NVOP_SIZE) { > - /* N-VOP */ > + if (s->b_frame_ref->size <= MAX_NVOP_SIZE) { > + /* N-VOP - discard stored data */ > av_log(ctx, AV_LOG_DEBUG, "Skipping N-VOP.\n"); > - } else { > - /* copy packet into BSFContext */ > - av_packet_move_ref(s->b_frame, in); > + av_buffer_unref(&s->b_frame_ref); > } > } else if (nb_vop >= 2) { > /* use first frame of the packet */ > - av_packet_move_ref(out, in); > - out->size = pos_vop2; > + pkt->size = pos_vop2; > } else if (pos_p >= 0) { > - ret = av_packet_make_writable(in); > + ret = av_packet_make_writable(pkt); > if (ret < 0) > goto fail; > av_log(ctx, AV_LOG_DEBUG, "Updating DivX userdata (remove trailing 'p').\n"); > - av_packet_move_ref(out, in); > /* remove 'p' (packed) from the end of the (DivX) userdata string */ > - out->data[pos_p] = '\0'; > + pkt->data[pos_p] = '\0'; > } else { > - /* copy packet */ > - av_packet_move_ref(out, in); > + /* use packet as is */ > } > > fail: > if (ret < 0) > - av_packet_unref(out); > - av_packet_free(&in); > + av_packet_unref(pkt); > > return ret; > } > > static int mpeg4_unpack_bframes_init(AVBSFContext *ctx) > { > - UnpackBFramesBSFContext *s = ctx->priv_data; > - > - s->b_frame = av_packet_alloc(); > - if (!s->b_frame) > - return AVERROR(ENOMEM); > - > if (ctx->par_in->extradata) { > int pos_p_ext = -1; > scan_buffer(ctx->par_in->extradata, ctx->par_in->extradata_size, &pos_p_ext, NULL, NULL); > @@ -155,13 +148,13 @@ static int mpeg4_unpack_bframes_init(AVBSFContext *ctx) > static void mpeg4_unpack_bframes_flush(AVBSFContext *bsfc) > { > UnpackBFramesBSFContext *ctx = bsfc->priv_data; > - av_packet_unref(ctx->b_frame); > + av_buffer_unref(&ctx->b_frame_ref); > } > > static void mpeg4_unpack_bframes_close(AVBSFContext *bsfc) > { > UnpackBFramesBSFContext *ctx = bsfc->priv_data; > - av_packet_free(&ctx->b_frame); > + av_buffer_unref(&ctx->b_frame_ref); > } > > static const enum AVCodecID codec_ids[] = { > Ping. - Andreas
Andreas Rheinhardt: > Andreas Rheinhardt: >> 1. Since bd90a2ec, mpeg4_unpack_bframes caches whole packets instead of >> just the pointer to the buffer and the buffer's size in order to be able >> to make use of refcounting to avoid copying of data; this unfortunately >> introduced copies of packet structures and side data (if existing), >> although the only fields that are needed are the buffer-related ones >> (data, size and buf). This can be changed without compromising the >> advantages of refcounting by storing a reference to the buffer. >> >> 2. This change also makes it easy to use only one packet throughout >> so that an allocation and free of an AVPacket structure per filtered >> packet can be saved by switching to ff_bsf_get_packet_ref. >> >> 3. Furthermore, this commit also fixes a memleak introduced in bd90a2ec: >> If a stored b_frame with side data was used for a later frame, the side >> data would leak when the input frame's properties were copied into the >> output frame. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavcodec/mpeg4_unpack_bframes_bsf.c | 77 ++++++++++++--------------- >> 1 file changed, 35 insertions(+), 42 deletions(-) >> >> diff --git a/libavcodec/mpeg4_unpack_bframes_bsf.c b/libavcodec/mpeg4_unpack_bframes_bsf.c >> index 1daf133ce5..382070b423 100644 >> --- a/libavcodec/mpeg4_unpack_bframes_bsf.c >> +++ b/libavcodec/mpeg4_unpack_bframes_bsf.c >> @@ -25,7 +25,7 @@ >> #include "mpeg4video.h" >> >> typedef struct UnpackBFramesBSFContext { >> - AVPacket *b_frame; >> + AVBufferRef *b_frame_ref; >> } UnpackBFramesBSFContext; >> >> /* determine the position of the packed marker in the userdata, >> @@ -56,32 +56,32 @@ static void scan_buffer(const uint8_t *buf, int buf_size, >> } >> } >> >> -static int mpeg4_unpack_bframes_filter(AVBSFContext *ctx, AVPacket *out) >> +static int mpeg4_unpack_bframes_filter(AVBSFContext *ctx, AVPacket *pkt) >> { >> UnpackBFramesBSFContext *s = ctx->priv_data; >> int pos_p = -1, nb_vop = 0, pos_vop2 = -1, ret = 0; >> - AVPacket *in; >> >> - ret = ff_bsf_get_packet(ctx, &in); >> + ret = ff_bsf_get_packet_ref(ctx, pkt); >> if (ret < 0) >> return ret; >> >> - scan_buffer(in->data, in->size, &pos_p, &nb_vop, &pos_vop2); >> + scan_buffer(pkt->data, pkt->size, &pos_p, &nb_vop, &pos_vop2); >> av_log(ctx, AV_LOG_DEBUG, "Found %d VOP startcode(s) in this packet.\n", nb_vop); >> >> if (pos_vop2 >= 0) { >> - if (s->b_frame->data) { >> + if (s->b_frame_ref) { >> av_log(ctx, AV_LOG_WARNING, >> "Missing one N-VOP packet, discarding one B-frame.\n"); >> - av_packet_unref(s->b_frame); >> + av_buffer_unref(&s->b_frame_ref); >> } >> - /* store the packed B-frame in the BSFContext */ >> - ret = av_packet_ref(s->b_frame, in); >> - if (ret < 0) { >> + /* store a reference to the packed B-frame's data in the BSFContext */ >> + s->b_frame_ref = av_buffer_ref(pkt->buf); >> + if (!s->b_frame_ref) { >> + ret = AVERROR(ENOMEM); >> goto fail; >> } >> - s->b_frame->size -= pos_vop2; >> - s->b_frame->data += pos_vop2; >> + s->b_frame_ref->data = pkt->data + pos_vop2; >> + s->b_frame_ref->size = pkt->size - pos_vop2; >> } >> >> if (nb_vop > 2) { >> @@ -89,56 +89,49 @@ static int mpeg4_unpack_bframes_filter(AVBSFContext *ctx, AVPacket *out) >> "Found %d VOP headers in one packet, only unpacking one.\n", nb_vop); >> } >> >> - if (nb_vop == 1 && s->b_frame->data) { >> - /* use frame from BSFContext */ >> - av_packet_move_ref(out, s->b_frame); >> + if (nb_vop == 1 && s->b_frame_ref) { >> + AVBufferRef *tmp = pkt->buf; >> >> - /* use properties from current input packet */ >> - ret = av_packet_copy_props(out, in); >> - if (ret < 0) { >> - goto fail; >> - } >> + /* make tmp accurately reflect the packet's data */ >> + tmp->data = pkt->data; >> + tmp->size = pkt->size; >> + >> + /* replace data in packet with stored data */ >> + pkt->buf = s->b_frame_ref; >> + pkt->data = s->b_frame_ref->data; >> + pkt->size = s->b_frame_ref->size; >> + >> + /* store reference to data into BSFContext */ >> + s->b_frame_ref = tmp; >> >> - if (in->size <= MAX_NVOP_SIZE) { >> - /* N-VOP */ >> + if (s->b_frame_ref->size <= MAX_NVOP_SIZE) { >> + /* N-VOP - discard stored data */ >> av_log(ctx, AV_LOG_DEBUG, "Skipping N-VOP.\n"); >> - } else { >> - /* copy packet into BSFContext */ >> - av_packet_move_ref(s->b_frame, in); >> + av_buffer_unref(&s->b_frame_ref); >> } >> } else if (nb_vop >= 2) { >> /* use first frame of the packet */ >> - av_packet_move_ref(out, in); >> - out->size = pos_vop2; >> + pkt->size = pos_vop2; >> } else if (pos_p >= 0) { >> - ret = av_packet_make_writable(in); >> + ret = av_packet_make_writable(pkt); >> if (ret < 0) >> goto fail; >> av_log(ctx, AV_LOG_DEBUG, "Updating DivX userdata (remove trailing 'p').\n"); >> - av_packet_move_ref(out, in); >> /* remove 'p' (packed) from the end of the (DivX) userdata string */ >> - out->data[pos_p] = '\0'; >> + pkt->data[pos_p] = '\0'; >> } else { >> - /* copy packet */ >> - av_packet_move_ref(out, in); >> + /* use packet as is */ >> } >> >> fail: >> if (ret < 0) >> - av_packet_unref(out); >> - av_packet_free(&in); >> + av_packet_unref(pkt); >> >> return ret; >> } >> >> static int mpeg4_unpack_bframes_init(AVBSFContext *ctx) >> { >> - UnpackBFramesBSFContext *s = ctx->priv_data; >> - >> - s->b_frame = av_packet_alloc(); >> - if (!s->b_frame) >> - return AVERROR(ENOMEM); >> - >> if (ctx->par_in->extradata) { >> int pos_p_ext = -1; >> scan_buffer(ctx->par_in->extradata, ctx->par_in->extradata_size, &pos_p_ext, NULL, NULL); >> @@ -155,13 +148,13 @@ static int mpeg4_unpack_bframes_init(AVBSFContext *ctx) >> static void mpeg4_unpack_bframes_flush(AVBSFContext *bsfc) >> { >> UnpackBFramesBSFContext *ctx = bsfc->priv_data; >> - av_packet_unref(ctx->b_frame); >> + av_buffer_unref(&ctx->b_frame_ref); >> } >> >> static void mpeg4_unpack_bframes_close(AVBSFContext *bsfc) >> { >> UnpackBFramesBSFContext *ctx = bsfc->priv_data; >> - av_packet_free(&ctx->b_frame); >> + av_buffer_unref(&ctx->b_frame_ref); >> } >> >> static const enum AVCodecID codec_ids[] = { >> > Ping. > > - Andreas > Another ping. - Andreas
Andreas Rheinhardt: > Andreas Rheinhardt: >> Andreas Rheinhardt: >>> 1. Since bd90a2ec, mpeg4_unpack_bframes caches whole packets instead of >>> just the pointer to the buffer and the buffer's size in order to be able >>> to make use of refcounting to avoid copying of data; this unfortunately >>> introduced copies of packet structures and side data (if existing), >>> although the only fields that are needed are the buffer-related ones >>> (data, size and buf). This can be changed without compromising the >>> advantages of refcounting by storing a reference to the buffer. >>> >>> 2. This change also makes it easy to use only one packet throughout >>> so that an allocation and free of an AVPacket structure per filtered >>> packet can be saved by switching to ff_bsf_get_packet_ref. >>> >>> 3. Furthermore, this commit also fixes a memleak introduced in bd90a2ec: >>> If a stored b_frame with side data was used for a later frame, the side >>> data would leak when the input frame's properties were copied into the >>> output frame. >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>> --- >>> libavcodec/mpeg4_unpack_bframes_bsf.c | 77 ++++++++++++--------------- >>> 1 file changed, 35 insertions(+), 42 deletions(-) >>> >>> diff --git a/libavcodec/mpeg4_unpack_bframes_bsf.c b/libavcodec/mpeg4_unpack_bframes_bsf.c >>> index 1daf133ce5..382070b423 100644 >>> --- a/libavcodec/mpeg4_unpack_bframes_bsf.c >>> +++ b/libavcodec/mpeg4_unpack_bframes_bsf.c >>> @@ -25,7 +25,7 @@ >>> #include "mpeg4video.h" >>> >>> typedef struct UnpackBFramesBSFContext { >>> - AVPacket *b_frame; >>> + AVBufferRef *b_frame_ref; >>> } UnpackBFramesBSFContext; >>> >>> /* determine the position of the packed marker in the userdata, >>> @@ -56,32 +56,32 @@ static void scan_buffer(const uint8_t *buf, int buf_size, >>> } >>> } >>> >>> -static int mpeg4_unpack_bframes_filter(AVBSFContext *ctx, AVPacket *out) >>> +static int mpeg4_unpack_bframes_filter(AVBSFContext *ctx, AVPacket *pkt) >>> { >>> UnpackBFramesBSFContext *s = ctx->priv_data; >>> int pos_p = -1, nb_vop = 0, pos_vop2 = -1, ret = 0; >>> - AVPacket *in; >>> >>> - ret = ff_bsf_get_packet(ctx, &in); >>> + ret = ff_bsf_get_packet_ref(ctx, pkt); >>> if (ret < 0) >>> return ret; >>> >>> - scan_buffer(in->data, in->size, &pos_p, &nb_vop, &pos_vop2); >>> + scan_buffer(pkt->data, pkt->size, &pos_p, &nb_vop, &pos_vop2); >>> av_log(ctx, AV_LOG_DEBUG, "Found %d VOP startcode(s) in this packet.\n", nb_vop); >>> >>> if (pos_vop2 >= 0) { >>> - if (s->b_frame->data) { >>> + if (s->b_frame_ref) { >>> av_log(ctx, AV_LOG_WARNING, >>> "Missing one N-VOP packet, discarding one B-frame.\n"); >>> - av_packet_unref(s->b_frame); >>> + av_buffer_unref(&s->b_frame_ref); >>> } >>> - /* store the packed B-frame in the BSFContext */ >>> - ret = av_packet_ref(s->b_frame, in); >>> - if (ret < 0) { >>> + /* store a reference to the packed B-frame's data in the BSFContext */ >>> + s->b_frame_ref = av_buffer_ref(pkt->buf); >>> + if (!s->b_frame_ref) { >>> + ret = AVERROR(ENOMEM); >>> goto fail; >>> } >>> - s->b_frame->size -= pos_vop2; >>> - s->b_frame->data += pos_vop2; >>> + s->b_frame_ref->data = pkt->data + pos_vop2; >>> + s->b_frame_ref->size = pkt->size - pos_vop2; >>> } >>> >>> if (nb_vop > 2) { >>> @@ -89,56 +89,49 @@ static int mpeg4_unpack_bframes_filter(AVBSFContext *ctx, AVPacket *out) >>> "Found %d VOP headers in one packet, only unpacking one.\n", nb_vop); >>> } >>> >>> - if (nb_vop == 1 && s->b_frame->data) { >>> - /* use frame from BSFContext */ >>> - av_packet_move_ref(out, s->b_frame); >>> + if (nb_vop == 1 && s->b_frame_ref) { >>> + AVBufferRef *tmp = pkt->buf; >>> >>> - /* use properties from current input packet */ >>> - ret = av_packet_copy_props(out, in); >>> - if (ret < 0) { >>> - goto fail; >>> - } >>> + /* make tmp accurately reflect the packet's data */ >>> + tmp->data = pkt->data; >>> + tmp->size = pkt->size; >>> + >>> + /* replace data in packet with stored data */ >>> + pkt->buf = s->b_frame_ref; >>> + pkt->data = s->b_frame_ref->data; >>> + pkt->size = s->b_frame_ref->size; >>> + >>> + /* store reference to data into BSFContext */ >>> + s->b_frame_ref = tmp; >>> >>> - if (in->size <= MAX_NVOP_SIZE) { >>> - /* N-VOP */ >>> + if (s->b_frame_ref->size <= MAX_NVOP_SIZE) { >>> + /* N-VOP - discard stored data */ >>> av_log(ctx, AV_LOG_DEBUG, "Skipping N-VOP.\n"); >>> - } else { >>> - /* copy packet into BSFContext */ >>> - av_packet_move_ref(s->b_frame, in); >>> + av_buffer_unref(&s->b_frame_ref); >>> } >>> } else if (nb_vop >= 2) { >>> /* use first frame of the packet */ >>> - av_packet_move_ref(out, in); >>> - out->size = pos_vop2; >>> + pkt->size = pos_vop2; >>> } else if (pos_p >= 0) { >>> - ret = av_packet_make_writable(in); >>> + ret = av_packet_make_writable(pkt); >>> if (ret < 0) >>> goto fail; >>> av_log(ctx, AV_LOG_DEBUG, "Updating DivX userdata (remove trailing 'p').\n"); >>> - av_packet_move_ref(out, in); >>> /* remove 'p' (packed) from the end of the (DivX) userdata string */ >>> - out->data[pos_p] = '\0'; >>> + pkt->data[pos_p] = '\0'; >>> } else { >>> - /* copy packet */ >>> - av_packet_move_ref(out, in); >>> + /* use packet as is */ >>> } >>> >>> fail: >>> if (ret < 0) >>> - av_packet_unref(out); >>> - av_packet_free(&in); >>> + av_packet_unref(pkt); >>> >>> return ret; >>> } >>> >>> static int mpeg4_unpack_bframes_init(AVBSFContext *ctx) >>> { >>> - UnpackBFramesBSFContext *s = ctx->priv_data; >>> - >>> - s->b_frame = av_packet_alloc(); >>> - if (!s->b_frame) >>> - return AVERROR(ENOMEM); >>> - >>> if (ctx->par_in->extradata) { >>> int pos_p_ext = -1; >>> scan_buffer(ctx->par_in->extradata, ctx->par_in->extradata_size, &pos_p_ext, NULL, NULL); >>> @@ -155,13 +148,13 @@ static int mpeg4_unpack_bframes_init(AVBSFContext *ctx) >>> static void mpeg4_unpack_bframes_flush(AVBSFContext *bsfc) >>> { >>> UnpackBFramesBSFContext *ctx = bsfc->priv_data; >>> - av_packet_unref(ctx->b_frame); >>> + av_buffer_unref(&ctx->b_frame_ref); >>> } >>> >>> static void mpeg4_unpack_bframes_close(AVBSFContext *bsfc) >>> { >>> UnpackBFramesBSFContext *ctx = bsfc->priv_data; >>> - av_packet_free(&ctx->b_frame); >>> + av_buffer_unref(&ctx->b_frame_ref); >>> } >>> >>> static const enum AVCodecID codec_ids[] = { >>> >> Ping. >> >> - Andreas >> > Another ping. > > - Andreas > And another ping. - Andreas
Could someone apply this? On 10/16/19, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > Andreas Rheinhardt: >> Andreas Rheinhardt: >>> Andreas Rheinhardt: >>>> 1. Since bd90a2ec, mpeg4_unpack_bframes caches whole packets instead of >>>> just the pointer to the buffer and the buffer's size in order to be able >>>> to make use of refcounting to avoid copying of data; this unfortunately >>>> introduced copies of packet structures and side data (if existing), >>>> although the only fields that are needed are the buffer-related ones >>>> (data, size and buf). This can be changed without compromising the >>>> advantages of refcounting by storing a reference to the buffer. >>>> >>>> 2. This change also makes it easy to use only one packet throughout >>>> so that an allocation and free of an AVPacket structure per filtered >>>> packet can be saved by switching to ff_bsf_get_packet_ref. >>>> >>>> 3. Furthermore, this commit also fixes a memleak introduced in bd90a2ec: >>>> If a stored b_frame with side data was used for a later frame, the side >>>> data would leak when the input frame's properties were copied into the >>>> output frame. >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>> --- >>>> libavcodec/mpeg4_unpack_bframes_bsf.c | 77 ++++++++++++--------------- >>>> 1 file changed, 35 insertions(+), 42 deletions(-) >>>> >>>> diff --git a/libavcodec/mpeg4_unpack_bframes_bsf.c >>>> b/libavcodec/mpeg4_unpack_bframes_bsf.c >>>> index 1daf133ce5..382070b423 100644 >>>> --- a/libavcodec/mpeg4_unpack_bframes_bsf.c >>>> +++ b/libavcodec/mpeg4_unpack_bframes_bsf.c >>>> @@ -25,7 +25,7 @@ >>>> #include "mpeg4video.h" >>>> >>>> typedef struct UnpackBFramesBSFContext { >>>> - AVPacket *b_frame; >>>> + AVBufferRef *b_frame_ref; >>>> } UnpackBFramesBSFContext; >>>> >>>> /* determine the position of the packed marker in the userdata, >>>> @@ -56,32 +56,32 @@ static void scan_buffer(const uint8_t *buf, int >>>> buf_size, >>>> } >>>> } >>>> >>>> -static int mpeg4_unpack_bframes_filter(AVBSFContext *ctx, AVPacket >>>> *out) >>>> +static int mpeg4_unpack_bframes_filter(AVBSFContext *ctx, AVPacket >>>> *pkt) >>>> { >>>> UnpackBFramesBSFContext *s = ctx->priv_data; >>>> int pos_p = -1, nb_vop = 0, pos_vop2 = -1, ret = 0; >>>> - AVPacket *in; >>>> >>>> - ret = ff_bsf_get_packet(ctx, &in); >>>> + ret = ff_bsf_get_packet_ref(ctx, pkt); >>>> if (ret < 0) >>>> return ret; >>>> >>>> - scan_buffer(in->data, in->size, &pos_p, &nb_vop, &pos_vop2); >>>> + scan_buffer(pkt->data, pkt->size, &pos_p, &nb_vop, &pos_vop2); >>>> av_log(ctx, AV_LOG_DEBUG, "Found %d VOP startcode(s) in this >>>> packet.\n", nb_vop); >>>> >>>> if (pos_vop2 >= 0) { >>>> - if (s->b_frame->data) { >>>> + if (s->b_frame_ref) { >>>> av_log(ctx, AV_LOG_WARNING, >>>> "Missing one N-VOP packet, discarding one >>>> B-frame.\n"); >>>> - av_packet_unref(s->b_frame); >>>> + av_buffer_unref(&s->b_frame_ref); >>>> } >>>> - /* store the packed B-frame in the BSFContext */ >>>> - ret = av_packet_ref(s->b_frame, in); >>>> - if (ret < 0) { >>>> + /* store a reference to the packed B-frame's data in the >>>> BSFContext */ >>>> + s->b_frame_ref = av_buffer_ref(pkt->buf); >>>> + if (!s->b_frame_ref) { >>>> + ret = AVERROR(ENOMEM); >>>> goto fail; >>>> } >>>> - s->b_frame->size -= pos_vop2; >>>> - s->b_frame->data += pos_vop2; >>>> + s->b_frame_ref->data = pkt->data + pos_vop2; >>>> + s->b_frame_ref->size = pkt->size - pos_vop2; >>>> } >>>> >>>> if (nb_vop > 2) { >>>> @@ -89,56 +89,49 @@ static int mpeg4_unpack_bframes_filter(AVBSFContext >>>> *ctx, AVPacket *out) >>>> "Found %d VOP headers in one packet, only unpacking one.\n", >>>> nb_vop); >>>> } >>>> >>>> - if (nb_vop == 1 && s->b_frame->data) { >>>> - /* use frame from BSFContext */ >>>> - av_packet_move_ref(out, s->b_frame); >>>> + if (nb_vop == 1 && s->b_frame_ref) { >>>> + AVBufferRef *tmp = pkt->buf; >>>> >>>> - /* use properties from current input packet */ >>>> - ret = av_packet_copy_props(out, in); >>>> - if (ret < 0) { >>>> - goto fail; >>>> - } >>>> + /* make tmp accurately reflect the packet's data */ >>>> + tmp->data = pkt->data; >>>> + tmp->size = pkt->size; >>>> + >>>> + /* replace data in packet with stored data */ >>>> + pkt->buf = s->b_frame_ref; >>>> + pkt->data = s->b_frame_ref->data; >>>> + pkt->size = s->b_frame_ref->size; >>>> + >>>> + /* store reference to data into BSFContext */ >>>> + s->b_frame_ref = tmp; >>>> >>>> - if (in->size <= MAX_NVOP_SIZE) { >>>> - /* N-VOP */ >>>> + if (s->b_frame_ref->size <= MAX_NVOP_SIZE) { >>>> + /* N-VOP - discard stored data */ >>>> av_log(ctx, AV_LOG_DEBUG, "Skipping N-VOP.\n"); >>>> - } else { >>>> - /* copy packet into BSFContext */ >>>> - av_packet_move_ref(s->b_frame, in); >>>> + av_buffer_unref(&s->b_frame_ref); >>>> } >>>> } else if (nb_vop >= 2) { >>>> /* use first frame of the packet */ >>>> - av_packet_move_ref(out, in); >>>> - out->size = pos_vop2; >>>> + pkt->size = pos_vop2; >>>> } else if (pos_p >= 0) { >>>> - ret = av_packet_make_writable(in); >>>> + ret = av_packet_make_writable(pkt); >>>> if (ret < 0) >>>> goto fail; >>>> av_log(ctx, AV_LOG_DEBUG, "Updating DivX userdata (remove >>>> trailing 'p').\n"); >>>> - av_packet_move_ref(out, in); >>>> /* remove 'p' (packed) from the end of the (DivX) userdata >>>> string */ >>>> - out->data[pos_p] = '\0'; >>>> + pkt->data[pos_p] = '\0'; >>>> } else { >>>> - /* copy packet */ >>>> - av_packet_move_ref(out, in); >>>> + /* use packet as is */ >>>> } >>>> >>>> fail: >>>> if (ret < 0) >>>> - av_packet_unref(out); >>>> - av_packet_free(&in); >>>> + av_packet_unref(pkt); >>>> >>>> return ret; >>>> } >>>> >>>> static int mpeg4_unpack_bframes_init(AVBSFContext *ctx) >>>> { >>>> - UnpackBFramesBSFContext *s = ctx->priv_data; >>>> - >>>> - s->b_frame = av_packet_alloc(); >>>> - if (!s->b_frame) >>>> - return AVERROR(ENOMEM); >>>> - >>>> if (ctx->par_in->extradata) { >>>> int pos_p_ext = -1; >>>> scan_buffer(ctx->par_in->extradata, >>>> ctx->par_in->extradata_size, &pos_p_ext, NULL, NULL); >>>> @@ -155,13 +148,13 @@ static int mpeg4_unpack_bframes_init(AVBSFContext >>>> *ctx) >>>> static void mpeg4_unpack_bframes_flush(AVBSFContext *bsfc) >>>> { >>>> UnpackBFramesBSFContext *ctx = bsfc->priv_data; >>>> - av_packet_unref(ctx->b_frame); >>>> + av_buffer_unref(&ctx->b_frame_ref); >>>> } >>>> >>>> static void mpeg4_unpack_bframes_close(AVBSFContext *bsfc) >>>> { >>>> UnpackBFramesBSFContext *ctx = bsfc->priv_data; >>>> - av_packet_free(&ctx->b_frame); >>>> + av_buffer_unref(&ctx->b_frame_ref); >>>> } >>>> >>>> static const enum AVCodecID codec_ids[] = { >>>> >>> Ping. >>> >>> - Andreas >>> >> Another ping. >> >> - Andreas >> > And another ping. > > - Andreas > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Wed, Oct 16, 2019 at 12:43:07PM +0200, Paul B Mahol wrote:
> Could someone apply this?
will apply
thx
[...]
Michael Niedermayer: > On Wed, Oct 16, 2019 at 12:43:07PM +0200, Paul B Mahol wrote: >> Could someone apply this? > > will apply > > thx > > [...] > Thanks. There is also a follow-up patch [1] to this that you have already LGTM'ed in July. Can you apply it, too, please? - Andreas [1]: https://patchwork.ffmpeg.org/patch/13891/
On Fri, Oct 18, 2019 at 04:44:00AM +0000, Andreas Rheinhardt wrote: > Michael Niedermayer: > > On Wed, Oct 16, 2019 at 12:43:07PM +0200, Paul B Mahol wrote: > >> Could someone apply this? > > > > will apply > > > > thx > > > > [...] > > > Thanks. There is also a follow-up patch [1] to this that you have > already LGTM'ed in July. Can you apply it, too, please? will do thx [...]
diff --git a/libavcodec/mpeg4_unpack_bframes_bsf.c b/libavcodec/mpeg4_unpack_bframes_bsf.c index 1daf133ce5..382070b423 100644 --- a/libavcodec/mpeg4_unpack_bframes_bsf.c +++ b/libavcodec/mpeg4_unpack_bframes_bsf.c @@ -25,7 +25,7 @@ #include "mpeg4video.h" typedef struct UnpackBFramesBSFContext { - AVPacket *b_frame; + AVBufferRef *b_frame_ref; } UnpackBFramesBSFContext; /* determine the position of the packed marker in the userdata, @@ -56,32 +56,32 @@ static void scan_buffer(const uint8_t *buf, int buf_size, } } -static int mpeg4_unpack_bframes_filter(AVBSFContext *ctx, AVPacket *out) +static int mpeg4_unpack_bframes_filter(AVBSFContext *ctx, AVPacket *pkt) { UnpackBFramesBSFContext *s = ctx->priv_data; int pos_p = -1, nb_vop = 0, pos_vop2 = -1, ret = 0; - AVPacket *in; - ret = ff_bsf_get_packet(ctx, &in); + ret = ff_bsf_get_packet_ref(ctx, pkt); if (ret < 0) return ret; - scan_buffer(in->data, in->size, &pos_p, &nb_vop, &pos_vop2); + scan_buffer(pkt->data, pkt->size, &pos_p, &nb_vop, &pos_vop2); av_log(ctx, AV_LOG_DEBUG, "Found %d VOP startcode(s) in this packet.\n", nb_vop); if (pos_vop2 >= 0) { - if (s->b_frame->data) { + if (s->b_frame_ref) { av_log(ctx, AV_LOG_WARNING, "Missing one N-VOP packet, discarding one B-frame.\n"); - av_packet_unref(s->b_frame); + av_buffer_unref(&s->b_frame_ref); } - /* store the packed B-frame in the BSFContext */ - ret = av_packet_ref(s->b_frame, in); - if (ret < 0) { + /* store a reference to the packed B-frame's data in the BSFContext */ + s->b_frame_ref = av_buffer_ref(pkt->buf); + if (!s->b_frame_ref) { + ret = AVERROR(ENOMEM); goto fail; } - s->b_frame->size -= pos_vop2; - s->b_frame->data += pos_vop2; + s->b_frame_ref->data = pkt->data + pos_vop2; + s->b_frame_ref->size = pkt->size - pos_vop2; } if (nb_vop > 2) { @@ -89,56 +89,49 @@ static int mpeg4_unpack_bframes_filter(AVBSFContext *ctx, AVPacket *out) "Found %d VOP headers in one packet, only unpacking one.\n", nb_vop); } - if (nb_vop == 1 && s->b_frame->data) { - /* use frame from BSFContext */ - av_packet_move_ref(out, s->b_frame); + if (nb_vop == 1 && s->b_frame_ref) { + AVBufferRef *tmp = pkt->buf; - /* use properties from current input packet */ - ret = av_packet_copy_props(out, in); - if (ret < 0) { - goto fail; - } + /* make tmp accurately reflect the packet's data */ + tmp->data = pkt->data; + tmp->size = pkt->size; + + /* replace data in packet with stored data */ + pkt->buf = s->b_frame_ref; + pkt->data = s->b_frame_ref->data; + pkt->size = s->b_frame_ref->size; + + /* store reference to data into BSFContext */ + s->b_frame_ref = tmp; - if (in->size <= MAX_NVOP_SIZE) { - /* N-VOP */ + if (s->b_frame_ref->size <= MAX_NVOP_SIZE) { + /* N-VOP - discard stored data */ av_log(ctx, AV_LOG_DEBUG, "Skipping N-VOP.\n"); - } else { - /* copy packet into BSFContext */ - av_packet_move_ref(s->b_frame, in); + av_buffer_unref(&s->b_frame_ref); } } else if (nb_vop >= 2) { /* use first frame of the packet */ - av_packet_move_ref(out, in); - out->size = pos_vop2; + pkt->size = pos_vop2; } else if (pos_p >= 0) { - ret = av_packet_make_writable(in); + ret = av_packet_make_writable(pkt); if (ret < 0) goto fail; av_log(ctx, AV_LOG_DEBUG, "Updating DivX userdata (remove trailing 'p').\n"); - av_packet_move_ref(out, in); /* remove 'p' (packed) from the end of the (DivX) userdata string */ - out->data[pos_p] = '\0'; + pkt->data[pos_p] = '\0'; } else { - /* copy packet */ - av_packet_move_ref(out, in); + /* use packet as is */ } fail: if (ret < 0) - av_packet_unref(out); - av_packet_free(&in); + av_packet_unref(pkt); return ret; } static int mpeg4_unpack_bframes_init(AVBSFContext *ctx) { - UnpackBFramesBSFContext *s = ctx->priv_data; - - s->b_frame = av_packet_alloc(); - if (!s->b_frame) - return AVERROR(ENOMEM); - if (ctx->par_in->extradata) { int pos_p_ext = -1; scan_buffer(ctx->par_in->extradata, ctx->par_in->extradata_size, &pos_p_ext, NULL, NULL); @@ -155,13 +148,13 @@ static int mpeg4_unpack_bframes_init(AVBSFContext *ctx) static void mpeg4_unpack_bframes_flush(AVBSFContext *bsfc) { UnpackBFramesBSFContext *ctx = bsfc->priv_data; - av_packet_unref(ctx->b_frame); + av_buffer_unref(&ctx->b_frame_ref); } static void mpeg4_unpack_bframes_close(AVBSFContext *bsfc) { UnpackBFramesBSFContext *ctx = bsfc->priv_data; - av_packet_free(&ctx->b_frame); + av_buffer_unref(&ctx->b_frame_ref); } static const enum AVCodecID codec_ids[] = {
1. Since bd90a2ec, mpeg4_unpack_bframes caches whole packets instead of just the pointer to the buffer and the buffer's size in order to be able to make use of refcounting to avoid copying of data; this unfortunately introduced copies of packet structures and side data (if existing), although the only fields that are needed are the buffer-related ones (data, size and buf). This can be changed without compromising the advantages of refcounting by storing a reference to the buffer. 2. This change also makes it easy to use only one packet throughout so that an allocation and free of an AVPacket structure per filtered packet can be saved by switching to ff_bsf_get_packet_ref. 3. Furthermore, this commit also fixes a memleak introduced in bd90a2ec: If a stored b_frame with side data was used for a later frame, the side data would leak when the input frame's properties were copied into the output frame. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavcodec/mpeg4_unpack_bframes_bsf.c | 77 ++++++++++++--------------- 1 file changed, 35 insertions(+), 42 deletions(-)