Re: [FFmpeg-devel] [PATCH 2/2] avcodec/noise_bsf: move the reference in the bsf internal buffer

[James Almer]

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

[James Almer]

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

[Michael Niedermayer]

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

[James Almer]

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

[Michael Niedermayer]

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