| Message ID | 20180316231601.3212-2-jamrial@gmail.com |
|---|---|
| State | Withdrawn |
| Headers | show |
On Fri, Mar 16, 2018 at 08:16:01PM -0300, James Almer wrote: > There's no need to allocate a new packet for it. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/noise_bsf.c | 30 ++++++++---------------------- > 1 file changed, 8 insertions(+), 22 deletions(-) should be ok assumung the the packet from ff_bsf_get_packet_ref can be modified, this is not documented in bsf.h thx [...]
On 3/17/2018 10:28 PM, Michael Niedermayer wrote: > On Fri, Mar 16, 2018 at 08:16:01PM -0300, James Almer wrote: >> There's no need to allocate a new packet for it. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/noise_bsf.c | 30 ++++++++---------------------- >> 1 file changed, 8 insertions(+), 22 deletions(-) > > should be ok assumung the the packet from ff_bsf_get_packet_ref can be > modified, this is not documented in bsf.h Yes, ff_bsf_get_packet_ref() moves the reference from the internal bsf packet as stored by av_bsf_send_packet() to the already allocated packet passed by the caller, whereas ff_bsf_get_packet() hands a newly allocated packet with the reference to the caller. > > thx > > [...] > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Sat, Mar 17, 2018 at 10:39:25PM -0300, James Almer wrote: > On 3/17/2018 10:28 PM, Michael Niedermayer wrote: > > On Fri, Mar 16, 2018 at 08:16:01PM -0300, James Almer wrote: > >> There's no need to allocate a new packet for it. > >> > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> libavcodec/noise_bsf.c | 30 ++++++++---------------------- > >> 1 file changed, 8 insertions(+), 22 deletions(-) > > > > should be ok assumung the the packet from ff_bsf_get_packet_ref can be > > modified, this is not documented in bsf.h > > Yes, ff_bsf_get_packet_ref() moves the reference from the internal bsf > packet as stored by av_bsf_send_packet() to the already allocated packet > passed by the caller, whereas ff_bsf_get_packet() hands a newly > allocated packet with the reference to the caller. what i meant is that the docs dont say that the caller can modify pkt->data without using some *make_writable() function also while ff_bsf_get_packet() documents freeing requirements, ff_bsf_get_packet_ref leaves this ambigous, from must over can to a must not. that is the docs should be made clearer assuming it is all correct [...]
On 3/17/2018 10:54 PM, Michael Niedermayer wrote: > On Sat, Mar 17, 2018 at 10:39:25PM -0300, James Almer wrote: >> On 3/17/2018 10:28 PM, Michael Niedermayer wrote: >>> On Fri, Mar 16, 2018 at 08:16:01PM -0300, James Almer wrote: >>>> There's no need to allocate a new packet for it. >>>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> libavcodec/noise_bsf.c | 30 ++++++++---------------------- >>>> 1 file changed, 8 insertions(+), 22 deletions(-) >>> >>> should be ok assumung the the packet from ff_bsf_get_packet_ref can be >>> modified, this is not documented in bsf.h >> >> Yes, ff_bsf_get_packet_ref() moves the reference from the internal bsf >> packet as stored by av_bsf_send_packet() to the already allocated packet >> passed by the caller, whereas ff_bsf_get_packet() hands a newly >> allocated packet with the reference to the caller. > > what i meant is that the docs dont say that the caller can > modify pkt->data without using some *make_writable() function Turns outs the packet passed to the caller is the same one with both functions. ff_bsf_get_packet() does tmp_pkt = av_packet_alloc(); if (!tmp_pkt) return AVERROR(ENOMEM); *pkt = ctx->internal->buffer_pkt; ctx->internal->buffer_pkt = tmp_pkt; Meaning it's not handing over a newly allocated packet but the buffered one instead. ff_bsf_get_packet_ref() in turn does av_packet_move_ref(pkt, ctx->internal->buffer_pkt); In both cases, ctx->internal->buffer_pkt is evidently expected to be writable by the time a bsf requests it, at least judging by how it's handled. See aac_adtstoasc, for example. I guess a make_writable() call in av_bsf_send_packet() would be a good idea to make sure that's effectively the case? Neither the doxy for av_bsf_send_packet() or ff_bsf_get_packet* mention anything about the writable status of this stored packet. > > also while ff_bsf_get_packet() documents freeing requirements, > ff_bsf_get_packet_ref leaves this ambigous, from must over can > to a must not. > > that is the docs should be made clearer assuming it is all correct > > [...] > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On 3/17/2018 11:13 PM, James Almer wrote: > On 3/17/2018 10:54 PM, Michael Niedermayer wrote: >> On Sat, Mar 17, 2018 at 10:39:25PM -0300, James Almer wrote: >>> On 3/17/2018 10:28 PM, Michael Niedermayer wrote: >>>> On Fri, Mar 16, 2018 at 08:16:01PM -0300, James Almer wrote: >>>>> There's no need to allocate a new packet for it. >>>>> >>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>> --- >>>>> libavcodec/noise_bsf.c | 30 ++++++++---------------------- >>>>> 1 file changed, 8 insertions(+), 22 deletions(-) >>>> >>>> should be ok assumung the the packet from ff_bsf_get_packet_ref can be >>>> modified, this is not documented in bsf.h >>> >>> Yes, ff_bsf_get_packet_ref() moves the reference from the internal bsf >>> packet as stored by av_bsf_send_packet() to the already allocated packet >>> passed by the caller, whereas ff_bsf_get_packet() hands a newly >>> allocated packet with the reference to the caller. >> >> what i meant is that the docs dont say that the caller can >> modify pkt->data without using some *make_writable() function > > Turns outs the packet passed to the caller is the same one with both > functions. > > ff_bsf_get_packet() does > > tmp_pkt = av_packet_alloc(); > if (!tmp_pkt) > return AVERROR(ENOMEM); > > *pkt = ctx->internal->buffer_pkt; > ctx->internal->buffer_pkt = tmp_pkt; > > Meaning it's not handing over a newly allocated packet but the buffered > one instead. > > ff_bsf_get_packet_ref() in turn does > > av_packet_move_ref(pkt, ctx->internal->buffer_pkt); > > In both cases, ctx->internal->buffer_pkt is evidently expected to be > writable by the time a bsf requests it, at least judging by how it's > handled. See aac_adtstoasc, for example. Actually no, aac_adtstoasc doesn't change the packet data, just the size and data pointer like many others, so nevermind. I'm dropping this patch, just in case. Sorry for the noise. > > I guess a make_writable() call in av_bsf_send_packet() would be a good > idea to make sure that's effectively the case? > Neither the doxy for av_bsf_send_packet() or ff_bsf_get_packet* mention > anything about the writable status of this stored packet. > >> >> also while ff_bsf_get_packet() documents freeing requirements, >> ff_bsf_get_packet_ref leaves this ambigous, from must over can >> to a must not. >> >> that is the docs should be made clearer assuming it is all correct >> >> [...] >> >> >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >
diff --git a/libavcodec/noise_bsf.c b/libavcodec/noise_bsf.c index 84b94032ad..b433cadccd 100644 --- a/libavcodec/noise_bsf.c +++ b/libavcodec/noise_bsf.c @@ -35,46 +35,32 @@ typedef struct NoiseContext { unsigned int state; } NoiseContext; -static int noise(AVBSFContext *ctx, AVPacket *out) +static int noise(AVBSFContext *ctx, AVPacket *pkt) { NoiseContext *s = ctx->priv_data; - AVPacket *in; int amount = s->amount > 0 ? s->amount : (s->state % 10001 + 1); int i, ret = 0; if (amount <= 0) return AVERROR(EINVAL); - ret = ff_bsf_get_packet(ctx, &in); + ret = ff_bsf_get_packet_ref(ctx, pkt); if (ret < 0) return ret; if (s->dropamount > 0 && s->state % s->dropamount == 0) { s->state++; - av_packet_free(&in); + av_packet_unref(pkt); return AVERROR(EAGAIN); } - ret = av_new_packet(out, in->size); - if (ret < 0) - goto fail; - - ret = av_packet_copy_props(out, in); - if (ret < 0) - goto fail; - - memcpy(out->data, in->data, in->size); - - for (i = 0; i < out->size; i++) { - s->state += out->data[i] + 1; + for (i = 0; i < pkt->size; i++) { + s->state += pkt->data[i] + 1; if (s->state % amount == 0) - out->data[i] = s->state; + pkt->data[i] = s->state; } -fail: - if (ret < 0) - av_packet_unref(out); - av_packet_free(&in); - return ret; + + return 0; } #define OFFSET(x) offsetof(NoiseContext, x)
There's no need to allocate a new packet for it. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/noise_bsf.c | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-)