[FFmpeg-devel] avcodec: Allow to query number of consumed bytes using the new API

This patch fixes the issue discussed in ticket #7708.

On 1/30/2019 12:53 PM, jannis_wth wrote:
> This patch fixes the issue discussed in ticket #7708.
> 
> 
> 0001-avcodec-Allow-to-query-number-of-consumed-bytes-usin.patch
> 
> From 756b3b59ac491bdbf01de4f399f5eeb74db8861a Mon Sep 17 00:00:00 2001
> From: user <user@host>
> Date: Wed, 30 Jan 2019 16:48:58 +0100
> Subject: [PATCH 1/1] avcodec: Allow to query number of consumed bytes using
>  the new API
> 
> Currently the only way to get the number of consumed bytes is by using
> the deprecated decode_audio4() API. This patch adds a simple function
> to fix this.
> 
> Signed-off-by: Jannis Kambs <jannis_wth@gmx.de>
> ---
>  libavcodec/avcodec.h | 9 +++++++++
>  libavcodec/utils.c   | 7 +++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index f554c53f0e..54f48754e4 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -5714,6 +5714,15 @@ int av_get_audio_frame_duration(AVCodecContext *avctx, int frame_bytes);
>   */
>  int av_get_audio_frame_duration2(AVCodecParameters *par, int frame_bytes);
>  
> +/**
> + * This function allows to get the number of consumed bytes, analogous to the
> + * old decode API. Call it after avcodec_receive_frame().
> + *
> + * @param avctx the codec context
> + * @return number of bytes consumed from the input AVPacket.
> + */
> +int av_get_num_consumed(AVCodecContext *avctx);

Use the avcodec_ prefix instead.

> +
>  #if FF_API_OLD_BSF
>  typedef struct AVBitStreamFilterContext {
>      void *priv_data;
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index d519b16092..a3cb8fef3f 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -1727,6 +1727,13 @@ int av_get_audio_frame_duration2(AVCodecParameters *par, int frame_bytes)
>                                      frame_bytes);
>  }
>  
> +int av_get_num_consumed(AVCodecContext *avctx)
> +{
> +    int ret = avctx->internal->compat_decode_consumed;
> +    avctx->internal->compat_decode_consumed = 0;

compat_decode_consumed doesn't look to have any significance when using
the new API. It's working as an accumulator for all bytes consumed from
all frames and it's never cleared (sounds like a potential overflow for
that matter in long lasting decoding scenarios like streaming), whereas
for the old API it's effectively keeping track of bytes consumed in one
frame, and cleared after every avcodec_decode_* call. So I guess that's
why you're clearing it here even though this function should by no means
do that.

Clearing compat_decode_consumed for the new API should be done somewhere
in decode.c where it doesn't break the logic for the old API, regardless
of using the AVCodec.decode() or AVCodec.receive_frame() callbacks.

> +    return ret;
> +}
> +
>  #if !HAVE_THREADS
>  int ff_thread_init(AVCodecContext *s)
>  {
> -- 2.11.0
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

James Almer (12019-01-30):
> compat_decode_consumed doesn't look to have any significance when using
> the new API. It's working as an accumulator for all bytes consumed from
> all frames and it's never cleared (sounds like a potential overflow for
> that matter in long lasting decoding scenarios like streaming), whereas
> for the old API it's effectively keeping track of bytes consumed in one
> frame, and cleared after every avcodec_decode_* call. So I guess that's
> why you're clearing it here even though this function should by no means
> do that.
> 
> Clearing compat_decode_consumed for the new API should be done somewhere
> in decode.c where it doesn't break the logic for the old API, regardless
> of using the AVCodec.decode() or AVCodec.receive_frame() callbacks.

I have reservations about adding an API to query something that is
considered deprecated.

Regards,

On 1/30/2019 2:19 PM, Nicolas George wrote:
> James Almer (12019-01-30):
>> compat_decode_consumed doesn't look to have any significance when using
>> the new API. It's working as an accumulator for all bytes consumed from
>> all frames and it's never cleared (sounds like a potential overflow for
>> that matter in long lasting decoding scenarios like streaming), whereas
>> for the old API it's effectively keeping track of bytes consumed in one
>> frame, and cleared after every avcodec_decode_* call. So I guess that's
>> why you're clearing it here even though this function should by no means
>> do that.
>>
>> Clearing compat_decode_consumed for the new API should be done somewhere
>> in decode.c where it doesn't break the logic for the old API, regardless
>> of using the AVCodec.decode() or AVCodec.receive_frame() callbacks.
> 
> I have reservations about adding an API to query something that is
> considered deprecated.
> 
> Regards,

I also don't think this is too good of an idea. The new API always
consumes and takes ownership of the full packet when you feed it to
avcodec_send_packet(), even if it ends up splitting it internally in
order to return more than one frame, so in theory just keeping track of
what you feed to it in your own code is enough, regardless of how many
frames avcodec_receive_frame() gives you back.

What this function would do for some decoders however is giving the API
user knowledge of said internal splitting after each
avcodec_receive_frame() call (an example being VP9).
If that's useful or not for the user, i don't know.

On 1/30/2019 3:43 PM, jannis_wth wrote:
> 30.01.19 18:30 - James Almer:
>> On 1/30/2019 2:19 PM, Nicolas George wrote:
>>> James Almer (12019-01-30):
>>>> compat_decode_consumed doesn't look to have any significance when using
>>>> the new API. It's working as an accumulator for all bytes consumed from
>>>> all frames and it's never cleared (sounds like a potential overflow for
>>>> that matter in long lasting decoding scenarios like streaming), whereas
>>>> for the old API it's effectively keeping track of bytes consumed in one
>>>> frame, and cleared after every avcodec_decode_* call. So I guess that's
>>>> why you're clearing it here even though this function should by no means
>>>> do that.
>>>>
>>>> Clearing compat_decode_consumed for the new API should be done somewhere
>>>> in decode.c where it doesn't break the logic for the old API, regardless
>>>> of using the AVCodec.decode() or AVCodec.receive_frame() callbacks.
>>> I have reservations about adding an API to query something that is
>>> considered deprecated.
>>>
>>> Regards,
>> I also don't think this is too good of an idea. The new API always
>> consumes and takes ownership of the full packet when you feed it to
>> avcodec_send_packet(), even if it ends up splitting it internally in
>> order to return more than one frame, so in theory just keeping track of
>> what you feed to it in your own code is enough, regardless of how many
>> frames avcodec_receive_frame() gives you back.
>>
>> What this function would do for some decoders however is giving the API
>> user knowledge of said internal splitting after each
>> avcodec_receive_frame() call (an example being VP9).
>> If that's useful or not for the user, i don't know.
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> Okay, so how about this one?
> This functionality is important if you don't know the packet size you
> are feeding. It allows you to reconstruct the size.

In what scenario you can't know the size of the AVPacket you're feeding
to avcodec_send_packet().

> 
> 
> 0001-avcodec-Allow-to-query-number-of-consumed-bytes-usin.patch
> 
> From bad739e4f5e9e78cc6fa799287d758bf27db8208 Mon Sep 17 00:00:00 2001
> From: user <user@host>
> Date: Wed, 30 Jan 2019 19:33:08 +0100
> Subject: [PATCH 1/1] avcodec: Allow to query number of consumed bytes using
>  the new API
> 
> Currently the only way to get the number of consumed bytes is by using
> the deprecated decode_audio4() API. This patch adds a simple function
> to fix this.
> 
> Signed-off-by: Jannis Kambs <jannis_wth@gmx.de>
> ---
>  libavcodec/avcodec.h | 9 +++++++++
>  libavcodec/utils.c   | 5 +++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index f554c53f0e..43bf84e58b 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -5714,6 +5714,15 @@ int av_get_audio_frame_duration(AVCodecContext *avctx, int frame_bytes);
>   */
>  int av_get_audio_frame_duration2(AVCodecParameters *par, int frame_bytes);
>  
> +/**
> + * This function allows to get the number of remaining bytes after
> + * avcodec_receive_frame() has been called.
> + *
> + * @param avctx the codec context
> + * @return number of remaining bytes from the input AVPacket.
> + */
> +int avcodec_get_remaining_size(AVCodecContext *avctx);
> +
>  #if FF_API_OLD_BSF
>  typedef struct AVBitStreamFilterContext {
>      void *priv_data;
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index d519b16092..638154f974 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -1727,6 +1727,11 @@ int av_get_audio_frame_duration2(AVCodecParameters *par, int frame_bytes)
>                                      frame_bytes);
>  }
>  
> +int avcodec_get_remaining_size(AVCodecContext *avctx)
> +{
> +    return avctx->internal->ds.in_pkt->size;

This is only used by decoders implementing the AVCodec.decode()
callback. It will always be zero for any decoder using
AVCodec.receive_frame(), like Bink Audio, libdav1d, etc.

For that matter, You sent this message to me alone. Make sure to send it
to the mailing list when you reply.

30.01.19 19:51 James Almer:
> On 1/30/2019 3:43 PM, jannis_wth wrote:
>> Okay, so how about this one?
>> This functionality is important if you don't know the packet size you
>> are feeding. It allows you to reconstruct the size.
> 
> In what scenario you can't know the size of the AVPacket you're feeding
> to avcodec_send_packet().
> 
For example when a mp4 container lost its moov atom.

>>
>> 0001-avcodec-Allow-to-query-number-of-consumed-bytes-usin.patch
>>
>> From bad739e4f5e9e78cc6fa799287d758bf27db8208 Mon Sep 17 00:00:00 2001
>> From: user <user@host>
>> Date: Wed, 30 Jan 2019 19:33:08 +0100
>> Subject: [PATCH 1/1] avcodec: Allow to query number of consumed bytes using
>>  the new API
>>
>> Currently the only way to get the number of consumed bytes is by using
>> the deprecated decode_audio4() API. This patch adds a simple function
>> to fix this.
>>
>> Signed-off-by: Jannis Kambs <jannis_wth@gmx.de>
>> ---
>>  libavcodec/avcodec.h | 9 +++++++++
>>  libavcodec/utils.c   | 5 +++++
>>  2 files changed, 14 insertions(+)
>>
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index f554c53f0e..43bf84e58b 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -5714,6 +5714,15 @@ int av_get_audio_frame_duration(AVCodecContext *avctx, int frame_bytes);
>>   */
>>  int av_get_audio_frame_duration2(AVCodecParameters *par, int frame_bytes);
>>  
>> +/**
>> + * This function allows to get the number of remaining bytes after
>> + * avcodec_receive_frame() has been called.
>> + *
>> + * @param avctx the codec context
>> + * @return number of remaining bytes from the input AVPacket.
>> + */
>> +int avcodec_get_remaining_size(AVCodecContext *avctx);
>> +
>>  #if FF_API_OLD_BSF
>>  typedef struct AVBitStreamFilterContext {
>>      void *priv_data;
>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> index d519b16092..638154f974 100644
>> --- a/libavcodec/utils.c
>> +++ b/libavcodec/utils.c
>> @@ -1727,6 +1727,11 @@ int av_get_audio_frame_duration2(AVCodecParameters *par, int frame_bytes)
>>                                      frame_bytes);
>>  }
>>  
>> +int avcodec_get_remaining_size(AVCodecContext *avctx)
>> +{
>> +    return avctx->internal->ds.in_pkt->size;
> 
> This is only used by decoders implementing the AVCodec.decode()
> callback. It will always be zero for any decoder using
> AVCodec.receive_frame(), like Bink Audio, libdav1d, etc.

Well, what would be the correct way then?
Or is there no good way and thus you won't accept such interface?

On 1/30/2019 4:19 PM, jannis_wth wrote:
> 30.01.19 19:51 James Almer:
>> On 1/30/2019 3:43 PM, jannis_wth wrote:
>>> Okay, so how about this one?
>>> This functionality is important if you don't know the packet size you
>>> are feeding. It allows you to reconstruct the size.
>>
>> In what scenario you can't know the size of the AVPacket you're feeding
>> to avcodec_send_packet().
>>
> For example when a mp4 container lost its moov atom.

What value is stored in pkt->size in this scenario at the time you feed
it to libavcodec? This function will return that value after all,
provided it's valid data.

Also, AVCodecParsers are the usual way to reconstruct/assemble broken or
incomplete packets.

> 
>>>
>>> 0001-avcodec-Allow-to-query-number-of-consumed-bytes-usin.patch
>>>
>>> From bad739e4f5e9e78cc6fa799287d758bf27db8208 Mon Sep 17 00:00:00 2001
>>> From: user <user@host>
>>> Date: Wed, 30 Jan 2019 19:33:08 +0100
>>> Subject: [PATCH 1/1] avcodec: Allow to query number of consumed bytes using
>>>  the new API
>>>
>>> Currently the only way to get the number of consumed bytes is by using
>>> the deprecated decode_audio4() API. This patch adds a simple function
>>> to fix this.
>>>
>>> Signed-off-by: Jannis Kambs <jannis_wth@gmx.de>
>>> ---
>>>  libavcodec/avcodec.h | 9 +++++++++
>>>  libavcodec/utils.c   | 5 +++++
>>>  2 files changed, 14 insertions(+)
>>>
>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>> index f554c53f0e..43bf84e58b 100644
>>> --- a/libavcodec/avcodec.h
>>> +++ b/libavcodec/avcodec.h
>>> @@ -5714,6 +5714,15 @@ int av_get_audio_frame_duration(AVCodecContext *avctx, int frame_bytes);
>>>   */
>>>  int av_get_audio_frame_duration2(AVCodecParameters *par, int frame_bytes);
>>>  
>>> +/**
>>> + * This function allows to get the number of remaining bytes after
>>> + * avcodec_receive_frame() has been called.
>>> + *
>>> + * @param avctx the codec context
>>> + * @return number of remaining bytes from the input AVPacket.
>>> + */
>>> +int avcodec_get_remaining_size(AVCodecContext *avctx);
>>> +
>>>  #if FF_API_OLD_BSF
>>>  typedef struct AVBitStreamFilterContext {
>>>      void *priv_data;
>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>>> index d519b16092..638154f974 100644
>>> --- a/libavcodec/utils.c
>>> +++ b/libavcodec/utils.c
>>> @@ -1727,6 +1727,11 @@ int av_get_audio_frame_duration2(AVCodecParameters *par, int frame_bytes)
>>>                                      frame_bytes);
>>>  }
>>>  
>>> +int avcodec_get_remaining_size(AVCodecContext *avctx)
>>> +{
>>> +    return avctx->internal->ds.in_pkt->size;
>>
>> This is only used by decoders implementing the AVCodec.decode()
>> callback. It will always be zero for any decoder using
>> AVCodec.receive_frame(), like Bink Audio, libdav1d, etc.
> 
> Well, what would be the correct way then?
> Or is there no good way and thus you won't accept such interface?

Accepting it or not depends on what other developers think about such a
function. Unlike the old API, the new one always fully consumes all
packets you feed to it, so there's no real need to check how many bytes
were consumed.
Trying to expose the internals for this purpose is not really feasible,
as you could see with all the different layers of internal buffering.

I guess avctx->internal->last_pkt_props could work for this, but it may
not be consistent across decoders.

30.01.19 21:02 - James Almer:
> On 1/30/2019 4:19 PM, jannis_wth wrote:
>> 30.01.19 19:51 James Almer:
>>> On 1/30/2019 3:43 PM, jannis_wth wrote:
>>>> Okay, so how about this one?
>>>> This functionality is important if you don't know the packet size you
>>>> are feeding. It allows you to reconstruct the size.
>>>
>>> In what scenario you can't know the size of the AVPacket you're feeding
>>> to avcodec_send_packet().
>>>
>> For example when a mp4 container lost its moov atom.
> 
> What value is stored in pkt->size in this scenario at the time you feed
> it to libavcodec? This function will return that value after all,
> provided it's valid data.
> 
> Also, AVCodecParsers are the usual way to reconstruct/assemble broken or
> incomplete packets.

pkt->size and its offset can be used to rebuild the moov atom.
I tried av_parser_parse2() but it I think it's not what I need. Its
return value does not match the true packet size, not even close.

>>> This is only used by decoders implementing the AVCodec.decode()
>>> callback. It will always be zero for any decoder using
>>> AVCodec.receive_frame(), like Bink Audio, libdav1d, etc.
>>
>> Well, what would be the correct way then?
>> Or is there no good way and thus you won't accept such interface?
> 
> Accepting it or not depends on what other developers think about such a
> function. Unlike the old API, the new one always fully consumes all
> packets you feed to it, so there's no real need to check how many bytes
> were consumed.
> Trying to expose the internals for this purpose is not really feasible,
> as you could see with all the different layers of internal buffering.
> 
> I guess avctx->internal->last_pkt_props could work for this, but it may
> not be consistent across decoders.

avctx->internal->last_pkt_props stores the properties of the last passed
packet, and does not get updated on decoding frames. Since I set the
input-packets size to a constant (size is unknown) this is not very
helpfull..

One could add a warning to avcodec_get_remaining_size() saying it only
works for some codecs.