| Message ID | 20190617034223.21195-2-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 1e93f5060f6f6f7a8729022d0120004902b4f64b |
| Headers | show |
On 6/17/2019 12:42 AM, Andreas Rheinhardt wrote: > Up until now, ff_cbs_write_packet always initialized the packet > structure it received without documenting this behaviour; furthermore, > the packet's buffer would (on success) be overwritten with the new > buffer without unreferencing the old. This meant that the input packet > had to be either clean (otherwise there would be memleaks) in which case > the initialization is redundant or uninitialized. ff_cbs_write_packet > was never used with uninitialized packets, so the initialization was > redundant. Worse yet, it forced callers to use more than one packet and > made it difficult to add side-data to a packet designated for output, > because said side-data could only be attached after the call to > ff_cbs_write_packet. > > This has been changed. It is now allowed to use a non-blank packet. > The currently existing buffer will be unreferenced and replaced by > the new one, as will be the accompanying fields (i.e. data and size). > The rest isn't touched at all. > > This change will enable us to use only one packet in the bitstream > filters that rely on CBS. > > This commit also updates the documentation of ff_cbs_write_extradata > and ff_cbs_write_packet (to better describe existing behaviour and in > the latter case to also describe the new behaviour). > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > I could also have made it unref the packet's buffer at the beginning; > this would have the advantage that the packet's buffer would be freed > after the units have been rewritten (if they are rewritten) and after > the fragment's buffer has been unreferenced, so that maximum memory > consumption would decrease. It would also be in line with all current > callers of ff_cbs_write_packet, but maybe it wouldn't be what a future > caller wants. What do you think? > libavcodec/cbs.c | 3 ++- > libavcodec/cbs.h | 10 +++++++++- > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c > index 0260ba6f67..47679eca1b 100644 > --- a/libavcodec/cbs.c > +++ b/libavcodec/cbs.c > @@ -357,7 +357,8 @@ int ff_cbs_write_packet(CodedBitstreamContext *ctx, > if (!buf) > return AVERROR(ENOMEM); > > - av_init_packet(pkt); > + av_buffer_unref(&pkt->buf); You should probably unref the packet, not just the AVBufferRef. > + > pkt->buf = buf; > pkt->data = frag->data; > pkt->size = frag->data_size; > diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h > index 967dcd1468..5260a39c63 100644 > --- a/libavcodec/cbs.h > +++ b/libavcodec/cbs.h > @@ -297,7 +297,8 @@ int ff_cbs_write_fragment_data(CodedBitstreamContext *ctx, > /** > * Write the bitstream of a fragment to the extradata in codec parameters. > * > - * This replaces any existing extradata in the structure. > + * Modifies context and fragment as ff_cbs_write_fragment_data does and > + * replaces any existing extradata in the structure. > */ > int ff_cbs_write_extradata(CodedBitstreamContext *ctx, > AVCodecParameters *par, > @@ -305,6 +306,13 @@ int ff_cbs_write_extradata(CodedBitstreamContext *ctx, > > /** > * Write the bitstream of a fragment to a packet. > + * > + * Modifies context and fragment as ff_cbs_write_fragment_data does. > + * > + * On success, the packet's buf is unreferenced and its buf, data and > + * size fields are set to the corresponding values from the newly updated > + * fragment; other fields are not touched. On failure, the packet is not > + * touched at all. > */ > int ff_cbs_write_packet(CodedBitstreamContext *ctx, > AVPacket *pkt, >
On 6/17/2019 9:44 AM, James Almer wrote: > On 6/17/2019 12:42 AM, Andreas Rheinhardt wrote: >> Up until now, ff_cbs_write_packet always initialized the packet >> structure it received without documenting this behaviour; furthermore, >> the packet's buffer would (on success) be overwritten with the new >> buffer without unreferencing the old. This meant that the input packet >> had to be either clean (otherwise there would be memleaks) in which case >> the initialization is redundant or uninitialized. ff_cbs_write_packet >> was never used with uninitialized packets, so the initialization was >> redundant. Worse yet, it forced callers to use more than one packet and >> made it difficult to add side-data to a packet designated for output, >> because said side-data could only be attached after the call to >> ff_cbs_write_packet. >> >> This has been changed. It is now allowed to use a non-blank packet. >> The currently existing buffer will be unreferenced and replaced by >> the new one, as will be the accompanying fields (i.e. data and size). >> The rest isn't touched at all. >> >> This change will enable us to use only one packet in the bitstream >> filters that rely on CBS. >> >> This commit also updates the documentation of ff_cbs_write_extradata >> and ff_cbs_write_packet (to better describe existing behaviour and in >> the latter case to also describe the new behaviour). >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> I could also have made it unref the packet's buffer at the beginning; >> this would have the advantage that the packet's buffer would be freed >> after the units have been rewritten (if they are rewritten) and after >> the fragment's buffer has been unreferenced, so that maximum memory >> consumption would decrease. It would also be in line with all current >> callers of ff_cbs_write_packet, but maybe it wouldn't be what a future >> caller wants. What do you think? >> libavcodec/cbs.c | 3 ++- >> libavcodec/cbs.h | 10 +++++++++- >> 2 files changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c >> index 0260ba6f67..47679eca1b 100644 >> --- a/libavcodec/cbs.c >> +++ b/libavcodec/cbs.c >> @@ -357,7 +357,8 @@ int ff_cbs_write_packet(CodedBitstreamContext *ctx, >> if (!buf) >> return AVERROR(ENOMEM); >> >> - av_init_packet(pkt); >> + av_buffer_unref(&pkt->buf); > > You should probably unref the packet, not just the AVBufferRef. Right, i see in patch 2 why you did this. I don't like much how with this change ff_cbs_write_packet would leave the packet in a weird state of having the filtered data alongside unrelated props already in packet provided by the caller. If CBS is ever made public, i'm not sure it's a behavior we should keep. But if right now it allows us to use ff_bsf_get_packet_ref() and remove av_packet_copy_props() calls, then it's good. > >> + >> pkt->buf = buf; >> pkt->data = frag->data; >> pkt->size = frag->data_size; >> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h >> index 967dcd1468..5260a39c63 100644 >> --- a/libavcodec/cbs.h >> +++ b/libavcodec/cbs.h >> @@ -297,7 +297,8 @@ int ff_cbs_write_fragment_data(CodedBitstreamContext *ctx, >> /** >> * Write the bitstream of a fragment to the extradata in codec parameters. >> * >> - * This replaces any existing extradata in the structure. >> + * Modifies context and fragment as ff_cbs_write_fragment_data does and >> + * replaces any existing extradata in the structure. >> */ >> int ff_cbs_write_extradata(CodedBitstreamContext *ctx, >> AVCodecParameters *par, >> @@ -305,6 +306,13 @@ int ff_cbs_write_extradata(CodedBitstreamContext *ctx, >> >> /** >> * Write the bitstream of a fragment to a packet. >> + * >> + * Modifies context and fragment as ff_cbs_write_fragment_data does. >> + * >> + * On success, the packet's buf is unreferenced and its buf, data and >> + * size fields are set to the corresponding values from the newly updated >> + * fragment; other fields are not touched. On failure, the packet is not >> + * touched at all. >> */ >> int ff_cbs_write_packet(CodedBitstreamContext *ctx, >> AVPacket *pkt, >> >
James Almer: > On 6/17/2019 9:44 AM, James Almer wrote: >> On 6/17/2019 12:42 AM, Andreas Rheinhardt wrote: >>> Up until now, ff_cbs_write_packet always initialized the packet >>> structure it received without documenting this behaviour; furthermore, >>> the packet's buffer would (on success) be overwritten with the new >>> buffer without unreferencing the old. This meant that the input packet >>> had to be either clean (otherwise there would be memleaks) in which case >>> the initialization is redundant or uninitialized. ff_cbs_write_packet >>> was never used with uninitialized packets, so the initialization was >>> redundant. Worse yet, it forced callers to use more than one packet and >>> made it difficult to add side-data to a packet designated for output, >>> because said side-data could only be attached after the call to >>> ff_cbs_write_packet. >>> >>> This has been changed. It is now allowed to use a non-blank packet. >>> The currently existing buffer will be unreferenced and replaced by >>> the new one, as will be the accompanying fields (i.e. data and size). >>> The rest isn't touched at all. >>> >>> This change will enable us to use only one packet in the bitstream >>> filters that rely on CBS. >>> >>> This commit also updates the documentation of ff_cbs_write_extradata >>> and ff_cbs_write_packet (to better describe existing behaviour and in >>> the latter case to also describe the new behaviour). >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>> --- >>> I could also have made it unref the packet's buffer at the beginning; >>> this would have the advantage that the packet's buffer would be freed >>> after the units have been rewritten (if they are rewritten) and after >>> the fragment's buffer has been unreferenced, so that maximum memory >>> consumption would decrease. It would also be in line with all current >>> callers of ff_cbs_write_packet, but maybe it wouldn't be what a future >>> caller wants. What do you think? >>> libavcodec/cbs.c | 3 ++- >>> libavcodec/cbs.h | 10 +++++++++- >>> 2 files changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c >>> index 0260ba6f67..47679eca1b 100644 >>> --- a/libavcodec/cbs.c >>> +++ b/libavcodec/cbs.c >>> @@ -357,7 +357,8 @@ int ff_cbs_write_packet(CodedBitstreamContext *ctx, >>> if (!buf) >>> return AVERROR(ENOMEM); >>> >>> - av_init_packet(pkt); >>> + av_buffer_unref(&pkt->buf); >> >> You should probably unref the packet, not just the AVBufferRef. > > Right, i see in patch 2 why you did this. I don't like much how with > this change ff_cbs_write_packet would leave the packet in a weird state > of having the filtered data alongside unrelated props already in packet > provided by the caller. If CBS is ever made public, i'm not sure it's a > behavior we should keep. > > But if right now it allows us to use ff_bsf_get_packet_ref() and remove > av_packet_copy_props() calls, then it's good. > Would you prefer if ff_cbs_write_packet gets renamed to ff_cbs_update_packet_data? Anyway, thanks for taking your time to review this. - Andreas
On 6/17/2019 11:34 AM, Andreas Rheinhardt wrote: > James Almer: >> On 6/17/2019 9:44 AM, James Almer wrote: >>> On 6/17/2019 12:42 AM, Andreas Rheinhardt wrote: >>>> Up until now, ff_cbs_write_packet always initialized the packet >>>> structure it received without documenting this behaviour; furthermore, >>>> the packet's buffer would (on success) be overwritten with the new >>>> buffer without unreferencing the old. This meant that the input packet >>>> had to be either clean (otherwise there would be memleaks) in which case >>>> the initialization is redundant or uninitialized. ff_cbs_write_packet >>>> was never used with uninitialized packets, so the initialization was >>>> redundant. Worse yet, it forced callers to use more than one packet and >>>> made it difficult to add side-data to a packet designated for output, >>>> because said side-data could only be attached after the call to >>>> ff_cbs_write_packet. >>>> >>>> This has been changed. It is now allowed to use a non-blank packet. >>>> The currently existing buffer will be unreferenced and replaced by >>>> the new one, as will be the accompanying fields (i.e. data and size). >>>> The rest isn't touched at all. >>>> >>>> This change will enable us to use only one packet in the bitstream >>>> filters that rely on CBS. >>>> >>>> This commit also updates the documentation of ff_cbs_write_extradata >>>> and ff_cbs_write_packet (to better describe existing behaviour and in >>>> the latter case to also describe the new behaviour). >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>>> --- >>>> I could also have made it unref the packet's buffer at the beginning; >>>> this would have the advantage that the packet's buffer would be freed >>>> after the units have been rewritten (if they are rewritten) and after >>>> the fragment's buffer has been unreferenced, so that maximum memory >>>> consumption would decrease. It would also be in line with all current >>>> callers of ff_cbs_write_packet, but maybe it wouldn't be what a future >>>> caller wants. What do you think? >>>> libavcodec/cbs.c | 3 ++- >>>> libavcodec/cbs.h | 10 +++++++++- >>>> 2 files changed, 11 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c >>>> index 0260ba6f67..47679eca1b 100644 >>>> --- a/libavcodec/cbs.c >>>> +++ b/libavcodec/cbs.c >>>> @@ -357,7 +357,8 @@ int ff_cbs_write_packet(CodedBitstreamContext *ctx, >>>> if (!buf) >>>> return AVERROR(ENOMEM); >>>> >>>> - av_init_packet(pkt); >>>> + av_buffer_unref(&pkt->buf); >>> >>> You should probably unref the packet, not just the AVBufferRef. >> >> Right, i see in patch 2 why you did this. I don't like much how with >> this change ff_cbs_write_packet would leave the packet in a weird state >> of having the filtered data alongside unrelated props already in packet >> provided by the caller. If CBS is ever made public, i'm not sure it's a >> behavior we should keep. >> >> But if right now it allows us to use ff_bsf_get_packet_ref() and remove >> av_packet_copy_props() calls, then it's good. >> > Would you prefer if ff_cbs_write_packet gets renamed to > ff_cbs_update_packet_data? > Anyway, thanks for taking your time to review this. No, it's fine as is. > > - 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". >
diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c index 0260ba6f67..47679eca1b 100644 --- a/libavcodec/cbs.c +++ b/libavcodec/cbs.c @@ -357,7 +357,8 @@ int ff_cbs_write_packet(CodedBitstreamContext *ctx, if (!buf) return AVERROR(ENOMEM); - av_init_packet(pkt); + av_buffer_unref(&pkt->buf); + pkt->buf = buf; pkt->data = frag->data; pkt->size = frag->data_size; diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h index 967dcd1468..5260a39c63 100644 --- a/libavcodec/cbs.h +++ b/libavcodec/cbs.h @@ -297,7 +297,8 @@ int ff_cbs_write_fragment_data(CodedBitstreamContext *ctx, /** * Write the bitstream of a fragment to the extradata in codec parameters. * - * This replaces any existing extradata in the structure. + * Modifies context and fragment as ff_cbs_write_fragment_data does and + * replaces any existing extradata in the structure. */ int ff_cbs_write_extradata(CodedBitstreamContext *ctx, AVCodecParameters *par, @@ -305,6 +306,13 @@ int ff_cbs_write_extradata(CodedBitstreamContext *ctx, /** * Write the bitstream of a fragment to a packet. + * + * Modifies context and fragment as ff_cbs_write_fragment_data does. + * + * On success, the packet's buf is unreferenced and its buf, data and + * size fields are set to the corresponding values from the newly updated + * fragment; other fields are not touched. On failure, the packet is not + * touched at all. */ int ff_cbs_write_packet(CodedBitstreamContext *ctx, AVPacket *pkt,
Up until now, ff_cbs_write_packet always initialized the packet structure it received without documenting this behaviour; furthermore, the packet's buffer would (on success) be overwritten with the new buffer without unreferencing the old. This meant that the input packet had to be either clean (otherwise there would be memleaks) in which case the initialization is redundant or uninitialized. ff_cbs_write_packet was never used with uninitialized packets, so the initialization was redundant. Worse yet, it forced callers to use more than one packet and made it difficult to add side-data to a packet designated for output, because said side-data could only be attached after the call to ff_cbs_write_packet. This has been changed. It is now allowed to use a non-blank packet. The currently existing buffer will be unreferenced and replaced by the new one, as will be the accompanying fields (i.e. data and size). The rest isn't touched at all. This change will enable us to use only one packet in the bitstream filters that rely on CBS. This commit also updates the documentation of ff_cbs_write_extradata and ff_cbs_write_packet (to better describe existing behaviour and in the latter case to also describe the new behaviour). Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- I could also have made it unref the packet's buffer at the beginning; this would have the advantage that the packet's buffer would be freed after the units have been rewritten (if they are rewritten) and after the fragment's buffer has been unreferenced, so that maximum memory consumption would decrease. It would also be in line with all current callers of ff_cbs_write_packet, but maybe it wouldn't be what a future caller wants. What do you think? libavcodec/cbs.c | 3 ++- libavcodec/cbs.h | 10 +++++++++- 2 files changed, 11 insertions(+), 2 deletions(-)