Re: [FFmpeg-devel] [PATCH] avcodec/bsf: make sure the AVBSFInternal stored packet is reference counted
On 3/23/18, James Almer wrote:
> Some bitstream filters may buffer said packet in their own contexts
> for latter use.
> The documentation for av_bsf_send_packet() doesn't forbid feeding
> it non-reference counted packets, which depending on the way said
> packets were internally buffered by the bsf it may result in the
> data described in them to become invalid or unavailable at any time.
>
> This was the case with vp9_superframe after commit e1bc3f4396, which
> was then promptly fixed in 37f4a093f7 and 7a02b364b6. It is still the
> case even today with vp9_reorder_raw.
>
> With this change the bitstream filters will not have to worry how to
> store or consume the packets fed to them.
>
> Signed-off-by: James Almer
> ---
> Regarding vp9_raw_reorder, see "[PATCH] avcodec/vp9_raw_reorder: cache
> input packets using new references" for a local fix similar to what
> vp9_superframe got with 37f4a093f7 and 7a02b364b6.
>
> A simple reproducer if you're curious is:
>
> ffmpeg -i INPUT -c:v copy -bsf:v vp9_raw_reorder -f null -
>
> Which segfauls with current git master.
>
> "[PATCH 2/2] ffmpeg: pass reference counted packets on codec copy
> when possible" also works around this in most cases by doing what its
> subject describes, but only affects the ffmpeg CLI only and not the
> API itself, of course.
>
> libavcodec/bsf.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
> index 38b423101c..25f7a20ad6 100644
> --- a/libavcodec/bsf.c
> +++ b/libavcodec/bsf.c
> @@ -188,7 +188,15 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket
> *pkt)
> ctx->internal->buffer_pkt->side_data_elems)
> return AVERROR(EAGAIN);
>
> -av_packet_move_ref(ctx->internal->buffer_pkt, pkt);
> +if (pkt->buf)
Use { } here and below.
> +av_packet_move_ref(ctx->internal->buffer_pkt, pkt);
> +else {
> +int ret = av_packet_ref(ctx->internal->buffer_pkt, pkt);
> +
> +if (ret < 0)
> +return ret;
> +av_packet_unref(pkt);
> +}
>
> return 0;
> }
> --
> 2.16.2
>
> ___
> 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] avcodec/bsf: make sure the AVBSFInternal stored packet is reference counted
On Fri, 23 Mar 2018 18:38:22 -0300
James Almer wrote:
> Some bitstream filters may buffer said packet in their own contexts
> for latter use.
> The documentation for av_bsf_send_packet() doesn't forbid feeding
> it non-reference counted packets, which depending on the way said
> packets were internally buffered by the bsf it may result in the
> data described in them to become invalid or unavailable at any time.
>
> This was the case with vp9_superframe after commit e1bc3f4396, which
> was then promptly fixed in 37f4a093f7 and 7a02b364b6. It is still the
> case even today with vp9_reorder_raw.
>
> With this change the bitstream filters will not have to worry how to
> store or consume the packets fed to them.
>
> Signed-off-by: James Almer
> ---
> Regarding vp9_raw_reorder, see "[PATCH] avcodec/vp9_raw_reorder: cache
> input packets using new references" for a local fix similar to what
> vp9_superframe got with 37f4a093f7 and 7a02b364b6.
>
> A simple reproducer if you're curious is:
>
> ffmpeg -i INPUT -c:v copy -bsf:v vp9_raw_reorder -f null -
>
> Which segfauls with current git master.
>
> "[PATCH 2/2] ffmpeg: pass reference counted packets on codec copy
> when possible" also works around this in most cases by doing what its
> subject describes, but only affects the ffmpeg CLI only and not the
> API itself, of course.
>
> libavcodec/bsf.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
> index 38b423101c..25f7a20ad6 100644
> --- a/libavcodec/bsf.c
> +++ b/libavcodec/bsf.c
> @@ -188,7 +188,15 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt)
> ctx->internal->buffer_pkt->side_data_elems)
> return AVERROR(EAGAIN);
>
> -av_packet_move_ref(ctx->internal->buffer_pkt, pkt);
> +if (pkt->buf)
> +av_packet_move_ref(ctx->internal->buffer_pkt, pkt);
> +else {
> +int ret = av_packet_ref(ctx->internal->buffer_pkt, pkt);
> +
> +if (ret < 0)
> +return ret;
> +av_packet_unref(pkt);
> +}
>
> return 0;
> }
Fine, but we should probably really provide an API function that
ensures that a packet is refcounted (and made refcounting if it isn't).
av_dup_packet() does this, but it's deprecated and has a bad name.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/bsf: make sure the AVBSFInternal stored packet is reference counted
On 3/23/2018 7:46 PM, wm4 wrote:
> On Fri, 23 Mar 2018 18:38:22 -0300
> James Almer wrote:
>
>> Some bitstream filters may buffer said packet in their own contexts
>> for latter use.
>> The documentation for av_bsf_send_packet() doesn't forbid feeding
>> it non-reference counted packets, which depending on the way said
>> packets were internally buffered by the bsf it may result in the
>> data described in them to become invalid or unavailable at any time.
>>
>> This was the case with vp9_superframe after commit e1bc3f4396, which
>> was then promptly fixed in 37f4a093f7 and 7a02b364b6. It is still the
>> case even today with vp9_reorder_raw.
>>
>> With this change the bitstream filters will not have to worry how to
>> store or consume the packets fed to them.
>>
>> Signed-off-by: James Almer
>> ---
>> Regarding vp9_raw_reorder, see "[PATCH] avcodec/vp9_raw_reorder: cache
>> input packets using new references" for a local fix similar to what
>> vp9_superframe got with 37f4a093f7 and 7a02b364b6.
>>
>> A simple reproducer if you're curious is:
>>
>> ffmpeg -i INPUT -c:v copy -bsf:v vp9_raw_reorder -f null -
>>
>> Which segfauls with current git master.
>>
>> "[PATCH 2/2] ffmpeg: pass reference counted packets on codec copy
>> when possible" also works around this in most cases by doing what its
>> subject describes, but only affects the ffmpeg CLI only and not the
>> API itself, of course.
>>
>> libavcodec/bsf.c | 10 +-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
>> index 38b423101c..25f7a20ad6 100644
>> --- a/libavcodec/bsf.c
>> +++ b/libavcodec/bsf.c
>> @@ -188,7 +188,15 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt)
>> ctx->internal->buffer_pkt->side_data_elems)
>> return AVERROR(EAGAIN);
>>
>> -av_packet_move_ref(ctx->internal->buffer_pkt, pkt);
>> +if (pkt->buf)
>> +av_packet_move_ref(ctx->internal->buffer_pkt, pkt);
>> +else {
>> +int ret = av_packet_ref(ctx->internal->buffer_pkt, pkt);
>> +
>> +if (ret < 0)
>> +return ret;
>> +av_packet_unref(pkt);
>> +}
>>
>> return 0;
>> }
>
> Fine, but we should probably really provide an API function that
> ensures that a packet is refcounted (and made refcounting if it isn't).
> av_dup_packet() does this, but it's deprecated and has a bad name.
av_packet_ref() ensures that, and so does av_packet_make_writable(), but
as a side effect of their main intended use. The documentation even
states to use av_packet_ref() for that purpose.
If you want one exactly like av_dup_packet() but in a less hacky way
that exclusively does "Make this packet's data ref counted if it's not,
do nothing else", we could add an av_packet_make_refcounted() function
or whatever. It should be trivial, so just tell me a name you'd like for
it and I'll write it.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/bsf: make sure the AVBSFInternal stored packet is reference counted
On 3/23/2018 8:15 PM, James Almer wrote:
> On 3/23/2018 7:46 PM, wm4 wrote:
>> On Fri, 23 Mar 2018 18:38:22 -0300
>> James Almer wrote:
>>
>>> Some bitstream filters may buffer said packet in their own contexts
>>> for latter use.
>>> The documentation for av_bsf_send_packet() doesn't forbid feeding
>>> it non-reference counted packets, which depending on the way said
>>> packets were internally buffered by the bsf it may result in the
>>> data described in them to become invalid or unavailable at any time.
>>>
>>> This was the case with vp9_superframe after commit e1bc3f4396, which
>>> was then promptly fixed in 37f4a093f7 and 7a02b364b6. It is still the
>>> case even today with vp9_reorder_raw.
>>>
>>> With this change the bitstream filters will not have to worry how to
>>> store or consume the packets fed to them.
>>>
>>> Signed-off-by: James Almer
>>> ---
>>> Regarding vp9_raw_reorder, see "[PATCH] avcodec/vp9_raw_reorder: cache
>>> input packets using new references" for a local fix similar to what
>>> vp9_superframe got with 37f4a093f7 and 7a02b364b6.
>>>
>>> A simple reproducer if you're curious is:
>>>
>>> ffmpeg -i INPUT -c:v copy -bsf:v vp9_raw_reorder -f null -
>>>
>>> Which segfauls with current git master.
>>>
>>> "[PATCH 2/2] ffmpeg: pass reference counted packets on codec copy
>>> when possible" also works around this in most cases by doing what its
>>> subject describes, but only affects the ffmpeg CLI only and not the
>>> API itself, of course.
>>>
>>> libavcodec/bsf.c | 10 +-
>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
>>> index 38b423101c..25f7a20ad6 100644
>>> --- a/libavcodec/bsf.c
>>> +++ b/libavcodec/bsf.c
>>> @@ -188,7 +188,15 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket
>>> *pkt)
>>> ctx->internal->buffer_pkt->side_data_elems)
>>> return AVERROR(EAGAIN);
>>>
>>> -av_packet_move_ref(ctx->internal->buffer_pkt, pkt);
>>> +if (pkt->buf)
>>> +av_packet_move_ref(ctx->internal->buffer_pkt, pkt);
>>> +else {
>>> +int ret = av_packet_ref(ctx->internal->buffer_pkt, pkt);
>>> +
>>> +if (ret < 0)
>>> +return ret;
>>> +av_packet_unref(pkt);
>>> +}
>>>
>>> return 0;
>>> }
>>
>> Fine, but we should probably really provide an API function that
>> ensures that a packet is refcounted (and made refcounting if it isn't).
>> av_dup_packet() does this, but it's deprecated and has a bad name.
>
> av_packet_ref() ensures that, and so does av_packet_make_writable(), but
> as a side effect of their main intended use. The documentation even
> states to use av_packet_ref() for that purpose.
>
> If you want one exactly like av_dup_packet() but in a less hacky way
> that exclusively does "Make this packet's data ref counted if it's not,
> do nothing else", we could add an av_packet_make_refcounted() function
> or whatever. It should be trivial, so just tell me a name you'd like for
> it and I'll write it.
Patch pushed in the meantime.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
