Re: [FFmpeg-devel] [PATCH 2/2] avcodec/noise_bsf: move the reference in the bsf internal buffer
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 > --- > 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 >> > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/noise_bsf: move the reference in the bsf internal buffer
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 --- 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 > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/noise_bsf: move the reference in the bsf internal buffer
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 > >> --- > >> 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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When you are offended at any man's fault, turn to yourself and study your own failings. Then you will forget your anger. -- Epictetus signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/noise_bsf: move the reference in the bsf internal buffer
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 >> --- >> 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 > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/noise_bsf: move the reference in the bsf internal buffer
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 > --- > 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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you drop bombs on a foreign country and kill a hundred thousand innocent people, expect your government to call the consequence "unprovoked inhuman terrorist attacks" and use it to justify dropping more bombs and killing more people. The technology changed, the idea is old. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
