diff mbox series

[FFmpeg-devel,v2] avcodec/bsf: restructure the internal implementation of the bistream filter API

Message ID 20200419143406.10834-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] avcodec/bsf: restructure the internal implementation of the bistream filter API
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

James Almer April 19, 2020, 2:34 p.m. UTC
Process input data as soon as it's fed to av_bsf_send_packet(), instead of
storing a single packet and expecting the user to call av_bsf_receive_packet()
in order to trigger the decoding process before they are allowed to feed more
data.

This puts the bsf API more in line with the decoupled decode API, without
breaking existing using it.

Signed-off-by: James Almer <jamrial@gmail.com>
---
Now without creating a new reference for the input packet, and behaving the
exact same as before for current users that were following the doxy right down
to the letter.

Also didn't rename buffer_pkt to reduce the size of the diff and easily find
the actual changes and additions.

 libavcodec/avcodec.h |  6 +-----
 libavcodec/bsf.c     | 42 +++++++++++++++++++++++++++++++-----------
 2 files changed, 32 insertions(+), 16 deletions(-)

Comments

Marton Balint April 19, 2020, 4:01 p.m. UTC | #1
On Sun, 19 Apr 2020, James Almer wrote:

> Process input data as soon as it's fed to av_bsf_send_packet(), instead of
> storing a single packet and expecting the user to call av_bsf_receive_packet()
> in order to trigger the decoding process before they are allowed to feed more
> data.
>
> This puts the bsf API more in line with the decoupled decode API, without
> breaking existing using it.

Well, previously it was assumed that av_bsf_send_packet() is never 
returning AVERROR_INVALIDDATA, that is no longer the case. That matters 
because current code in mux.c ingnores av_bsf_receive_packet() errors but 
not av_bsf_send_packet() errors.

Unless there is some benefit I am not seeing, I'd rather keep things as 
is. Sorry.

Regards,
Marton


>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Now without creating a new reference for the input packet, and behaving the
> exact same as before for current users that were following the doxy right down
> to the letter.
>
> Also didn't rename buffer_pkt to reduce the size of the diff and easily find
> the actual changes and additions.
>
> libavcodec/avcodec.h |  6 +-----
> libavcodec/bsf.c     | 42 +++++++++++++++++++++++++++++++-----------
> 2 files changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index b79b025e53..af2a1b0e90 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -4735,10 +4735,6 @@ int av_bsf_init(AVBSFContext *ctx);
> /**
>  * Submit a packet for filtering.
>  *
> - * After sending each packet, the filter must be completely drained by calling
> - * av_bsf_receive_packet() repeatedly until it returns AVERROR(EAGAIN) or
> - * AVERROR_EOF.
> - *
>  * @param pkt the packet to filter. The bitstream filter will take ownership of
>  * the packet and reset the contents of pkt. pkt is not touched if an error occurs.
>  * If pkt is empty (i.e. NULL, or pkt->data is NULL and pkt->side_data_elems zero),
> @@ -4770,7 +4766,7 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt);
>  * an error occurs.
>  *
>  * @note one input packet may result in several output packets, so after sending
> - * a packet with av_bsf_send_packet(), this function needs to be called
> + * a packet with av_bsf_send_packet(), this function may need to be called
>  * repeatedly until it stops returning 0. It is also possible for a filter to
>  * output fewer packets than were sent to it, so this function may return
>  * AVERROR(EAGAIN) immediately after a successful av_bsf_send_packet() call.
> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
> index b9fc771a88..c79a8ebbdf 100644
> --- a/libavcodec/bsf.c
> +++ b/libavcodec/bsf.c
> @@ -30,6 +30,7 @@
> 
> struct AVBSFInternal {
>     AVPacket *buffer_pkt;
> +    AVPacket *out_pkt;
>     int eof;
> };
> 
> @@ -46,8 +47,10 @@ void av_bsf_free(AVBSFContext **pctx)
>     if (ctx->filter->priv_class && ctx->priv_data)
>         av_opt_free(ctx->priv_data);
> 
> -    if (ctx->internal)
> +    if (ctx->internal) {
>         av_packet_free(&ctx->internal->buffer_pkt);
> +        av_packet_free(&ctx->internal->out_pkt);
> +    }
>     av_freep(&ctx->internal);
>     av_freep(&ctx->priv_data);
> 
> @@ -112,7 +115,8 @@ int av_bsf_alloc(const AVBitStreamFilter *filter, AVBSFContext **pctx)
>     ctx->internal = bsfi;
>
>     bsfi->buffer_pkt = av_packet_alloc();
> -    if (!bsfi->buffer_pkt) {
> +    bsfi->out_pkt = av_packet_alloc();
> +    if (!bsfi->buffer_pkt || !bsfi->out_pkt) {
>         ret = AVERROR(ENOMEM);
>         goto fail;
>     }
> @@ -185,6 +189,7 @@ void av_bsf_flush(AVBSFContext *ctx)
>     bsfi->eof = 0;
>
>     av_packet_unref(bsfi->buffer_pkt);
> +    av_packet_unref(bsfi->out_pkt);
>
>     if (ctx->filter->flush)
>         ctx->filter->flush(ctx);
> @@ -205,10 +210,21 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt)
>         return AVERROR(EINVAL);
>     }
> 
> +    if (!bsfi->buffer_pkt->data &&
> +        !bsfi->buffer_pkt->side_data_elems)
> +        goto end;
> +
> +    if (!bsfi->out_pkt->data && !bsfi->out_pkt->side_data_elems) {
> +        ret = ctx->filter->filter(ctx, bsfi->out_pkt);
> +        if (ret < 0 && ret != AVERROR(EAGAIN) && ret != AVERROR_EOF)
> +            return ret;
> +    }
> +
>     if (bsfi->buffer_pkt->data ||
>         bsfi->buffer_pkt->side_data_elems)
>         return AVERROR(EAGAIN);
> 
> +end:
>     ret = av_packet_make_refcounted(pkt);
>     if (ret < 0)
>         return ret;
> @@ -219,7 +235,17 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt)
> 
> int av_bsf_receive_packet(AVBSFContext *ctx, AVPacket *pkt)
> {
> -    return ctx->filter->filter(ctx, pkt);
> +    AVBSFInternal *bsfi = ctx->internal;
> +    int ret;
> +
> +    if (!bsfi->out_pkt->data && !bsfi->out_pkt->side_data_elems) {
> +        ret = ctx->filter->filter(ctx, bsfi->out_pkt);
> +        if (ret < 0)
> +            return ret;
> +    }
> +    av_packet_move_ref(pkt, bsfi->out_pkt);
> +
> +    return 0;
> }
> 
> int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt)
> @@ -227,12 +253,9 @@ int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt)
>     AVBSFInternal *bsfi = ctx->internal;
>     AVPacket *tmp_pkt;
> 
> -    if (bsfi->eof)
> -        return AVERROR_EOF;
> -
>     if (!bsfi->buffer_pkt->data &&
>         !bsfi->buffer_pkt->side_data_elems)
> -        return AVERROR(EAGAIN);
> +        return bsfi->eof ? AVERROR_EOF : AVERROR(EAGAIN);
>
>     tmp_pkt = av_packet_alloc();
>     if (!tmp_pkt)
> @@ -248,12 +271,9 @@ int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket *pkt)
> {
>     AVBSFInternal *bsfi = ctx->internal;
> 
> -    if (bsfi->eof)
> -        return AVERROR_EOF;
> -
>     if (!bsfi->buffer_pkt->data &&
>         !bsfi->buffer_pkt->side_data_elems)
> -        return AVERROR(EAGAIN);
> +        return bsfi->eof ? AVERROR_EOF : AVERROR(EAGAIN);
>
>     av_packet_move_ref(pkt, bsfi->buffer_pkt);
> 
> -- 
> 2.26.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer April 19, 2020, 4:42 p.m. UTC | #2
On 4/19/2020 1:01 PM, Marton Balint wrote:
> 
> 
> On Sun, 19 Apr 2020, James Almer wrote:
> 
>> Process input data as soon as it's fed to av_bsf_send_packet(),
>> instead of
>> storing a single packet and expecting the user to call
>> av_bsf_receive_packet()
>> in order to trigger the decoding process before they are allowed to
>> feed more
>> data.
>>
>> This puts the bsf API more in line with the decoupled decode API, without
>> breaking existing using it.
> 
> Well, previously it was assumed that av_bsf_send_packet() is never
> returning AVERROR_INVALIDDATA, that is no longer the case. That matters
> because current code in mux.c ingnores av_bsf_receive_packet() errors
> but not av_bsf_send_packet() errors.

The doxy states av_bsf_send_packet() can return an error, even if up
till now it hasn't.

Also, as i stated in the addendum below the Signed-off line, current
users following the recommended API usage (one call to
av_bsf_send_packet(), then several to av_bsf_receive_packet()) will see
no changes at all. So if you did that and expected no errors from
av_bsf_send_packet() in your mux.c code, you'll still get none.

> 
> Unless there is some benefit I am not seeing, I'd rather keep things as
> is. Sorry.

The benefit is relaxing the current constrains of the API for new users
(including mux.c if you want to take advantage of it), but for those
that don't care about it, there's no difference at all. So any existing
code is in no risk of being broken.

> 
> Regards,
> Marton
> 
> 
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> Now without creating a new reference for the input packet, and
>> behaving the
>> exact same as before for current users that were following the doxy
>> right down
>> to the letter.
>>
>> Also didn't rename buffer_pkt to reduce the size of the diff and
>> easily find
>> the actual changes and additions.
>>
>> libavcodec/avcodec.h |  6 +-----
>> libavcodec/bsf.c     | 42 +++++++++++++++++++++++++++++++-----------
>> 2 files changed, 32 insertions(+), 16 deletions(-)
>>
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index b79b025e53..af2a1b0e90 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -4735,10 +4735,6 @@ int av_bsf_init(AVBSFContext *ctx);
>> /**
>>  * Submit a packet for filtering.
>>  *
>> - * After sending each packet, the filter must be completely drained
>> by calling
>> - * av_bsf_receive_packet() repeatedly until it returns
>> AVERROR(EAGAIN) or
>> - * AVERROR_EOF.
>> - *
>>  * @param pkt the packet to filter. The bitstream filter will take
>> ownership of
>>  * the packet and reset the contents of pkt. pkt is not touched if an
>> error occurs.
>>  * If pkt is empty (i.e. NULL, or pkt->data is NULL and
>> pkt->side_data_elems zero),
>> @@ -4770,7 +4766,7 @@ int av_bsf_send_packet(AVBSFContext *ctx,
>> AVPacket *pkt);
>>  * an error occurs.
>>  *
>>  * @note one input packet may result in several output packets, so
>> after sending
>> - * a packet with av_bsf_send_packet(), this function needs to be called
>> + * a packet with av_bsf_send_packet(), this function may need to be
>> called
>>  * repeatedly until it stops returning 0. It is also possible for a
>> filter to
>>  * output fewer packets than were sent to it, so this function may return
>>  * AVERROR(EAGAIN) immediately after a successful av_bsf_send_packet()
>> call.
>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
>> index b9fc771a88..c79a8ebbdf 100644
>> --- a/libavcodec/bsf.c
>> +++ b/libavcodec/bsf.c
>> @@ -30,6 +30,7 @@
>>
>> struct AVBSFInternal {
>>     AVPacket *buffer_pkt;
>> +    AVPacket *out_pkt;
>>     int eof;
>> };
>>
>> @@ -46,8 +47,10 @@ void av_bsf_free(AVBSFContext **pctx)
>>     if (ctx->filter->priv_class && ctx->priv_data)
>>         av_opt_free(ctx->priv_data);
>>
>> -    if (ctx->internal)
>> +    if (ctx->internal) {
>>         av_packet_free(&ctx->internal->buffer_pkt);
>> +        av_packet_free(&ctx->internal->out_pkt);
>> +    }
>>     av_freep(&ctx->internal);
>>     av_freep(&ctx->priv_data);
>>
>> @@ -112,7 +115,8 @@ int av_bsf_alloc(const AVBitStreamFilter *filter,
>> AVBSFContext **pctx)
>>     ctx->internal = bsfi;
>>
>>     bsfi->buffer_pkt = av_packet_alloc();
>> -    if (!bsfi->buffer_pkt) {
>> +    bsfi->out_pkt = av_packet_alloc();
>> +    if (!bsfi->buffer_pkt || !bsfi->out_pkt) {
>>         ret = AVERROR(ENOMEM);
>>         goto fail;
>>     }
>> @@ -185,6 +189,7 @@ void av_bsf_flush(AVBSFContext *ctx)
>>     bsfi->eof = 0;
>>
>>     av_packet_unref(bsfi->buffer_pkt);
>> +    av_packet_unref(bsfi->out_pkt);
>>
>>     if (ctx->filter->flush)
>>         ctx->filter->flush(ctx);
>> @@ -205,10 +210,21 @@ int av_bsf_send_packet(AVBSFContext *ctx,
>> AVPacket *pkt)
>>         return AVERROR(EINVAL);
>>     }
>>
>> +    if (!bsfi->buffer_pkt->data &&
>> +        !bsfi->buffer_pkt->side_data_elems)
>> +        goto end;
>> +
>> +    if (!bsfi->out_pkt->data && !bsfi->out_pkt->side_data_elems) {
>> +        ret = ctx->filter->filter(ctx, bsfi->out_pkt);
>> +        if (ret < 0 && ret != AVERROR(EAGAIN) && ret != AVERROR_EOF)
>> +            return ret;
>> +    }
>> +
>>     if (bsfi->buffer_pkt->data ||
>>         bsfi->buffer_pkt->side_data_elems)
>>         return AVERROR(EAGAIN);
>>
>> +end:
>>     ret = av_packet_make_refcounted(pkt);
>>     if (ret < 0)
>>         return ret;
>> @@ -219,7 +235,17 @@ int av_bsf_send_packet(AVBSFContext *ctx,
>> AVPacket *pkt)
>>
>> int av_bsf_receive_packet(AVBSFContext *ctx, AVPacket *pkt)
>> {
>> -    return ctx->filter->filter(ctx, pkt);
>> +    AVBSFInternal *bsfi = ctx->internal;
>> +    int ret;
>> +
>> +    if (!bsfi->out_pkt->data && !bsfi->out_pkt->side_data_elems) {
>> +        ret = ctx->filter->filter(ctx, bsfi->out_pkt);
>> +        if (ret < 0)
>> +            return ret;
>> +    }
>> +    av_packet_move_ref(pkt, bsfi->out_pkt);
>> +
>> +    return 0;
>> }
>>
>> int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt)
>> @@ -227,12 +253,9 @@ int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket
>> **pkt)
>>     AVBSFInternal *bsfi = ctx->internal;
>>     AVPacket *tmp_pkt;
>>
>> -    if (bsfi->eof)
>> -        return AVERROR_EOF;
>> -
>>     if (!bsfi->buffer_pkt->data &&
>>         !bsfi->buffer_pkt->side_data_elems)
>> -        return AVERROR(EAGAIN);
>> +        return bsfi->eof ? AVERROR_EOF : AVERROR(EAGAIN);
>>
>>     tmp_pkt = av_packet_alloc();
>>     if (!tmp_pkt)
>> @@ -248,12 +271,9 @@ int ff_bsf_get_packet_ref(AVBSFContext *ctx,
>> AVPacket *pkt)
>> {
>>     AVBSFInternal *bsfi = ctx->internal;
>>
>> -    if (bsfi->eof)
>> -        return AVERROR_EOF;
>> -
>>     if (!bsfi->buffer_pkt->data &&
>>         !bsfi->buffer_pkt->side_data_elems)
>> -        return AVERROR(EAGAIN);
>> +        return bsfi->eof ? AVERROR_EOF : AVERROR(EAGAIN);
>>
>>     av_packet_move_ref(pkt, bsfi->buffer_pkt);
>>
>> -- 
>> 2.26.0
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer April 19, 2020, 4:48 p.m. UTC | #3
On 4/19/2020 11:34 AM, James Almer wrote:
> Process input data as soon as it's fed to av_bsf_send_packet(), instead of
> storing a single packet and expecting the user to call av_bsf_receive_packet()
> in order to trigger the decoding process before they are allowed to feed more
> data.

This is no longer the case in v2, sorry for the confusion.

av_bsf_send_packet() will trigger filtering only when called more than
one time in a row (When it can't consume the new packet without trying
to filter the previous one first), to keep behavior for existing API
users intact.

> 
> This puts the bsf API more in line with the decoupled decode API, without
> breaking existing using it.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Now without creating a new reference for the input packet, and behaving the
> exact same as before for current users that were following the doxy right down
> to the letter.
> 
> Also didn't rename buffer_pkt to reduce the size of the diff and easily find
> the actual changes and additions.
> 
>  libavcodec/avcodec.h |  6 +-----
>  libavcodec/bsf.c     | 42 +++++++++++++++++++++++++++++++-----------
>  2 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index b79b025e53..af2a1b0e90 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -4735,10 +4735,6 @@ int av_bsf_init(AVBSFContext *ctx);
>  /**
>   * Submit a packet for filtering.
>   *
> - * After sending each packet, the filter must be completely drained by calling
> - * av_bsf_receive_packet() repeatedly until it returns AVERROR(EAGAIN) or
> - * AVERROR_EOF.
> - *
>   * @param pkt the packet to filter. The bitstream filter will take ownership of
>   * the packet and reset the contents of pkt. pkt is not touched if an error occurs.
>   * If pkt is empty (i.e. NULL, or pkt->data is NULL and pkt->side_data_elems zero),
> @@ -4770,7 +4766,7 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt);
>   * an error occurs.
>   *
>   * @note one input packet may result in several output packets, so after sending
> - * a packet with av_bsf_send_packet(), this function needs to be called
> + * a packet with av_bsf_send_packet(), this function may need to be called
>   * repeatedly until it stops returning 0. It is also possible for a filter to
>   * output fewer packets than were sent to it, so this function may return
>   * AVERROR(EAGAIN) immediately after a successful av_bsf_send_packet() call.
> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
> index b9fc771a88..c79a8ebbdf 100644
> --- a/libavcodec/bsf.c
> +++ b/libavcodec/bsf.c
> @@ -30,6 +30,7 @@
>  
>  struct AVBSFInternal {
>      AVPacket *buffer_pkt;
> +    AVPacket *out_pkt;
>      int eof;
>  };
>  
> @@ -46,8 +47,10 @@ void av_bsf_free(AVBSFContext **pctx)
>      if (ctx->filter->priv_class && ctx->priv_data)
>          av_opt_free(ctx->priv_data);
>  
> -    if (ctx->internal)
> +    if (ctx->internal) {
>          av_packet_free(&ctx->internal->buffer_pkt);
> +        av_packet_free(&ctx->internal->out_pkt);
> +    }
>      av_freep(&ctx->internal);
>      av_freep(&ctx->priv_data);
>  
> @@ -112,7 +115,8 @@ int av_bsf_alloc(const AVBitStreamFilter *filter, AVBSFContext **pctx)
>      ctx->internal = bsfi;
>  
>      bsfi->buffer_pkt = av_packet_alloc();
> -    if (!bsfi->buffer_pkt) {
> +    bsfi->out_pkt = av_packet_alloc();
> +    if (!bsfi->buffer_pkt || !bsfi->out_pkt) {
>          ret = AVERROR(ENOMEM);
>          goto fail;
>      }
> @@ -185,6 +189,7 @@ void av_bsf_flush(AVBSFContext *ctx)
>      bsfi->eof = 0;
>  
>      av_packet_unref(bsfi->buffer_pkt);
> +    av_packet_unref(bsfi->out_pkt);
>  
>      if (ctx->filter->flush)
>          ctx->filter->flush(ctx);
> @@ -205,10 +210,21 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt)
>          return AVERROR(EINVAL);
>      }
>  
> +    if (!bsfi->buffer_pkt->data &&
> +        !bsfi->buffer_pkt->side_data_elems)
> +        goto end;
> +
> +    if (!bsfi->out_pkt->data && !bsfi->out_pkt->side_data_elems) {
> +        ret = ctx->filter->filter(ctx, bsfi->out_pkt);
> +        if (ret < 0 && ret != AVERROR(EAGAIN) && ret != AVERROR_EOF)
> +            return ret;
> +    }
> +
>      if (bsfi->buffer_pkt->data ||
>          bsfi->buffer_pkt->side_data_elems)
>          return AVERROR(EAGAIN);
>  
> +end:
>      ret = av_packet_make_refcounted(pkt);
>      if (ret < 0)
>          return ret;
> @@ -219,7 +235,17 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt)
>  
>  int av_bsf_receive_packet(AVBSFContext *ctx, AVPacket *pkt)
>  {
> -    return ctx->filter->filter(ctx, pkt);
> +    AVBSFInternal *bsfi = ctx->internal;
> +    int ret;
> +
> +    if (!bsfi->out_pkt->data && !bsfi->out_pkt->side_data_elems) {
> +        ret = ctx->filter->filter(ctx, bsfi->out_pkt);
> +        if (ret < 0)
> +            return ret;
> +    }
> +    av_packet_move_ref(pkt, bsfi->out_pkt);
> +
> +    return 0;
>  }
>  
>  int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt)
> @@ -227,12 +253,9 @@ int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt)
>      AVBSFInternal *bsfi = ctx->internal;
>      AVPacket *tmp_pkt;
>  
> -    if (bsfi->eof)
> -        return AVERROR_EOF;
> -
>      if (!bsfi->buffer_pkt->data &&
>          !bsfi->buffer_pkt->side_data_elems)
> -        return AVERROR(EAGAIN);
> +        return bsfi->eof ? AVERROR_EOF : AVERROR(EAGAIN);
>  
>      tmp_pkt = av_packet_alloc();
>      if (!tmp_pkt)
> @@ -248,12 +271,9 @@ int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket *pkt)
>  {
>      AVBSFInternal *bsfi = ctx->internal;
>  
> -    if (bsfi->eof)
> -        return AVERROR_EOF;
> -
>      if (!bsfi->buffer_pkt->data &&
>          !bsfi->buffer_pkt->side_data_elems)
> -        return AVERROR(EAGAIN);
> +        return bsfi->eof ? AVERROR_EOF : AVERROR(EAGAIN);
>  
>      av_packet_move_ref(pkt, bsfi->buffer_pkt);
>  
>
Marton Balint April 19, 2020, 6:26 p.m. UTC | #4
On Sun, 19 Apr 2020, James Almer wrote:

> On 4/19/2020 1:01 PM, Marton Balint wrote:
>> 
>> 
>> On Sun, 19 Apr 2020, James Almer wrote:
>> 
>>> Process input data as soon as it's fed to av_bsf_send_packet(),
>>> instead of
>>> storing a single packet and expecting the user to call
>>> av_bsf_receive_packet()
>>> in order to trigger the decoding process before they are allowed to
>>> feed more
>>> data.
>>>
>>> This puts the bsf API more in line with the decoupled decode API, without
>>> breaking existing using it.
>> 
>> Well, previously it was assumed that av_bsf_send_packet() is never
>> returning AVERROR_INVALIDDATA, that is no longer the case. That matters
>> because current code in mux.c ingnores av_bsf_receive_packet() errors
>> but not av_bsf_send_packet() errors.
>
> The doxy states av_bsf_send_packet() can return an error, even if up
> till now it hasn't.
>
> Also, as i stated in the addendum below the Signed-off line, current
> users following the recommended API usage (one call to
> av_bsf_send_packet(), then several to av_bsf_receive_packet()) will see
> no changes at all. So if you did that and expected no errors from
> av_bsf_send_packet() in your mux.c code, you'll still get none.
>
>> 
>> Unless there is some benefit I am not seeing, I'd rather keep things as
>> is. Sorry.
>
> The benefit is relaxing the current constrains of the API for new users
> (including mux.c if you want to take advantage of it),

How is this making things easier for the API user? After the patch, the 
framework can - in most cases - buffer 2 packets instead of 1. So what? 
User app still have to implement that if it gets EAGAIN in send_packet, it 
has to call receive_packet and vica versa. Unless it uses the 
"recommended" practice, where it can assume success of send_packet for a 
completetly drained bsf.

You say that the API should be the same as for encode/decode. Well, it 
already is. Decode/encode API makes no guarantees about the number of 
possibly buffered frames/packets. It only says that one of successive 
send/receive calls must succeed and not return EAGAIN. And that is true 
with the current BSF implementation.

You can remove the comment that "the filter must be completely drained", 
or change it to that it is recommended to completetly drain the bsf. But 
the code need no changes, it will still be API-compatible with the decode 
API. It just uses internally a stricter scheme, and I like that it does.

Regards,
Marton

> that don't care about it, there's no difference at all. So any existing 
> code is in no risk of being broken.
>
>> 
>> Regards,
>> Marton
>> 
>> 
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>> Now without creating a new reference for the input packet, and
>>> behaving the
>>> exact same as before for current users that were following the doxy
>>> right down
>>> to the letter.
>>>
>>> Also didn't rename buffer_pkt to reduce the size of the diff and
>>> easily find
>>> the actual changes and additions.
>>>
>>> libavcodec/avcodec.h |  6 +-----
>>> libavcodec/bsf.c     | 42 +++++++++++++++++++++++++++++++-----------
>>> 2 files changed, 32 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>> index b79b025e53..af2a1b0e90 100644
>>> --- a/libavcodec/avcodec.h
>>> +++ b/libavcodec/avcodec.h
>>> @@ -4735,10 +4735,6 @@ int av_bsf_init(AVBSFContext *ctx);
>>> /**
>>>  * Submit a packet for filtering.
>>>  *
>>> - * After sending each packet, the filter must be completely drained
>>> by calling
>>> - * av_bsf_receive_packet() repeatedly until it returns
>>> AVERROR(EAGAIN) or
>>> - * AVERROR_EOF.
>>> - *
>>>  * @param pkt the packet to filter. The bitstream filter will take
>>> ownership of
>>>  * the packet and reset the contents of pkt. pkt is not touched if an
>>> error occurs.
>>>  * If pkt is empty (i.e. NULL, or pkt->data is NULL and
>>> pkt->side_data_elems zero),
>>> @@ -4770,7 +4766,7 @@ int av_bsf_send_packet(AVBSFContext *ctx,
>>> AVPacket *pkt);
>>>  * an error occurs.
>>>  *
>>>  * @note one input packet may result in several output packets, so
>>> after sending
>>> - * a packet with av_bsf_send_packet(), this function needs to be called
>>> + * a packet with av_bsf_send_packet(), this function may need to be
>>> called
>>>  * repeatedly until it stops returning 0. It is also possible for a
>>> filter to
>>>  * output fewer packets than were sent to it, so this function may return
>>>  * AVERROR(EAGAIN) immediately after a successful av_bsf_send_packet()
>>> call.
>>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
>>> index b9fc771a88..c79a8ebbdf 100644
>>> --- a/libavcodec/bsf.c
>>> +++ b/libavcodec/bsf.c
>>> @@ -30,6 +30,7 @@
>>>
>>> struct AVBSFInternal {
>>>     AVPacket *buffer_pkt;
>>> +    AVPacket *out_pkt;
>>>     int eof;
>>> };
>>>
>>> @@ -46,8 +47,10 @@ void av_bsf_free(AVBSFContext **pctx)
>>>     if (ctx->filter->priv_class && ctx->priv_data)
>>>         av_opt_free(ctx->priv_data);
>>>
>>> -    if (ctx->internal)
>>> +    if (ctx->internal) {
>>>         av_packet_free(&ctx->internal->buffer_pkt);
>>> +        av_packet_free(&ctx->internal->out_pkt);
>>> +    }
>>>     av_freep(&ctx->internal);
>>>     av_freep(&ctx->priv_data);
>>>
>>> @@ -112,7 +115,8 @@ int av_bsf_alloc(const AVBitStreamFilter *filter,
>>> AVBSFContext **pctx)
>>>     ctx->internal = bsfi;
>>>
>>>     bsfi->buffer_pkt = av_packet_alloc();
>>> -    if (!bsfi->buffer_pkt) {
>>> +    bsfi->out_pkt = av_packet_alloc();
>>> +    if (!bsfi->buffer_pkt || !bsfi->out_pkt) {
>>>         ret = AVERROR(ENOMEM);
>>>         goto fail;
>>>     }
>>> @@ -185,6 +189,7 @@ void av_bsf_flush(AVBSFContext *ctx)
>>>     bsfi->eof = 0;
>>>
>>>     av_packet_unref(bsfi->buffer_pkt);
>>> +    av_packet_unref(bsfi->out_pkt);
>>>
>>>     if (ctx->filter->flush)
>>>         ctx->filter->flush(ctx);
>>> @@ -205,10 +210,21 @@ int av_bsf_send_packet(AVBSFContext *ctx,
>>> AVPacket *pkt)
>>>         return AVERROR(EINVAL);
>>>     }
>>>
>>> +    if (!bsfi->buffer_pkt->data &&
>>> +        !bsfi->buffer_pkt->side_data_elems)
>>> +        goto end;
>>> +
>>> +    if (!bsfi->out_pkt->data && !bsfi->out_pkt->side_data_elems) {
>>> +        ret = ctx->filter->filter(ctx, bsfi->out_pkt);
>>> +        if (ret < 0 && ret != AVERROR(EAGAIN) && ret != AVERROR_EOF)
>>> +            return ret;
>>> +    }
>>> +
>>>     if (bsfi->buffer_pkt->data ||
>>>         bsfi->buffer_pkt->side_data_elems)
>>>         return AVERROR(EAGAIN);
>>>
>>> +end:
>>>     ret = av_packet_make_refcounted(pkt);
>>>     if (ret < 0)
>>>         return ret;
>>> @@ -219,7 +235,17 @@ int av_bsf_send_packet(AVBSFContext *ctx,
>>> AVPacket *pkt)
>>>
>>> int av_bsf_receive_packet(AVBSFContext *ctx, AVPacket *pkt)
>>> {
>>> -    return ctx->filter->filter(ctx, pkt);
>>> +    AVBSFInternal *bsfi = ctx->internal;
>>> +    int ret;
>>> +
>>> +    if (!bsfi->out_pkt->data && !bsfi->out_pkt->side_data_elems) {
>>> +        ret = ctx->filter->filter(ctx, bsfi->out_pkt);
>>> +        if (ret < 0)
>>> +            return ret;
>>> +    }
>>> +    av_packet_move_ref(pkt, bsfi->out_pkt);
>>> +
>>> +    return 0;
>>> }
>>>
>>> int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt)
>>> @@ -227,12 +253,9 @@ int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket
>>> **pkt)
>>>     AVBSFInternal *bsfi = ctx->internal;
>>>     AVPacket *tmp_pkt;
>>>
>>> -    if (bsfi->eof)
>>> -        return AVERROR_EOF;
>>> -
>>>     if (!bsfi->buffer_pkt->data &&
>>>         !bsfi->buffer_pkt->side_data_elems)
>>> -        return AVERROR(EAGAIN);
>>> +        return bsfi->eof ? AVERROR_EOF : AVERROR(EAGAIN);
>>>
>>>     tmp_pkt = av_packet_alloc();
>>>     if (!tmp_pkt)
>>> @@ -248,12 +271,9 @@ int ff_bsf_get_packet_ref(AVBSFContext *ctx,
>>> AVPacket *pkt)
>>> {
>>>     AVBSFInternal *bsfi = ctx->internal;
>>>
>>> -    if (bsfi->eof)
>>> -        return AVERROR_EOF;
>>> -
>>>     if (!bsfi->buffer_pkt->data &&
>>>         !bsfi->buffer_pkt->side_data_elems)
>>> -        return AVERROR(EAGAIN);
>>> +        return bsfi->eof ? AVERROR_EOF : AVERROR(EAGAIN);
>>>
>>>     av_packet_move_ref(pkt, bsfi->buffer_pkt);
>>>
>>> -- 
>>> 2.26.0
>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> 
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer April 19, 2020, 6:47 p.m. UTC | #5
On 4/19/2020 3:26 PM, Marton Balint wrote:
> 
> 
> On Sun, 19 Apr 2020, James Almer wrote:
> 
>> On 4/19/2020 1:01 PM, Marton Balint wrote:
>>>
>>>
>>> On Sun, 19 Apr 2020, James Almer wrote:
>>>
>>>> Process input data as soon as it's fed to av_bsf_send_packet(),
>>>> instead of
>>>> storing a single packet and expecting the user to call
>>>> av_bsf_receive_packet()
>>>> in order to trigger the decoding process before they are allowed to
>>>> feed more
>>>> data.
>>>>
>>>> This puts the bsf API more in line with the decoupled decode API,
>>>> without
>>>> breaking existing using it.
>>>
>>> Well, previously it was assumed that av_bsf_send_packet() is never
>>> returning AVERROR_INVALIDDATA, that is no longer the case. That matters
>>> because current code in mux.c ingnores av_bsf_receive_packet() errors
>>> but not av_bsf_send_packet() errors.
>>
>> The doxy states av_bsf_send_packet() can return an error, even if up
>> till now it hasn't.
>>
>> Also, as i stated in the addendum below the Signed-off line, current
>> users following the recommended API usage (one call to
>> av_bsf_send_packet(), then several to av_bsf_receive_packet()) will see
>> no changes at all. So if you did that and expected no errors from
>> av_bsf_send_packet() in your mux.c code, you'll still get none.
>>
>>>
>>> Unless there is some benefit I am not seeing, I'd rather keep things as
>>> is. Sorry.
>>
>> The benefit is relaxing the current constrains of the API for new users
>> (including mux.c if you want to take advantage of it),
> 
> How is this making things easier for the API user? After the patch, the
> framework can - in most cases - buffer 2 packets instead of 1. So what?
> User app still have to implement that if it gets EAGAIN in send_packet,
> it has to call receive_packet and vica versa. Unless it uses the
> "recommended" practice, where it can assume success of send_packet for a
> completetly drained bsf.

The difference is that i can now call send_packet as many times as the
underlying bsf lets me, much like the decode send_packet, which allows
me to build the loop in many different ways and not just the currently
enforced "Send one packet, receive packets until drained" one.

> 
> You say that the API should be the same as for encode/decode. Well, it
> already is.

It certainly isn't. Unlike the decode API, I can't feed the
av1_frame_merge bsf all the packets it needs before it has one ready for
output. I need to feed them one, interleaved with at least one
receive_packet call that will tell me that i need send more before it
can output anything.

> Decode/encode API makes no guarantees about the number of
> possibly buffered frames/packets. It only says that one of successive
> send/receive calls must succeed and not return EAGAIN. And that is true
> with the current BSF implementation.
> 
> You can remove the comment that "the filter must be completely drained",
> or change it to that it is recommended to completetly drain the bsf. But
> the code need no changes, it will still be API-compatible with the
> decode API. It just uses internally a stricter scheme, and I like that
> it does.

I'm removing the stricter scheme without altering the workflow of users
of said strict scheme. It's not breaking your mux.c code, right? Or at
least it's not meant to if you followed the current strict guidelines.
So what exactly are you against of?

Instead of "Send one packet, receive packets until drained even if there
was nothing ready", i can now do "Send as many packets as I'm allowed
to, fetch one, try to send as many packets as i'm allowed to again,
fetch one, repeat", or "Send as many packets as i'm allowed to, fetch
one, send one, fetch one, repeat", or "Send as many packets as i'm
allowed to, fetch as many packets as there are available, repeat".

> 
> Regards,
> Marton
> 
>> that don't care about it, there's no difference at all. So any
>> existing code is in no risk of being broken.
>>
>>>
>>> Regards,
>>> Marton
>>>
>>>
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>> Now without creating a new reference for the input packet, and
>>>> behaving the
>>>> exact same as before for current users that were following the doxy
>>>> right down
>>>> to the letter.
>>>>
>>>> Also didn't rename buffer_pkt to reduce the size of the diff and
>>>> easily find
>>>> the actual changes and additions.
>>>>
>>>> libavcodec/avcodec.h |  6 +-----
>>>> libavcodec/bsf.c     | 42 +++++++++++++++++++++++++++++++-----------
>>>> 2 files changed, 32 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>>> index b79b025e53..af2a1b0e90 100644
>>>> --- a/libavcodec/avcodec.h
>>>> +++ b/libavcodec/avcodec.h
>>>> @@ -4735,10 +4735,6 @@ int av_bsf_init(AVBSFContext *ctx);
>>>> /**
>>>>  * Submit a packet for filtering.
>>>>  *
>>>> - * After sending each packet, the filter must be completely drained
>>>> by calling
>>>> - * av_bsf_receive_packet() repeatedly until it returns
>>>> AVERROR(EAGAIN) or
>>>> - * AVERROR_EOF.
>>>> - *
>>>>  * @param pkt the packet to filter. The bitstream filter will take
>>>> ownership of
>>>>  * the packet and reset the contents of pkt. pkt is not touched if an
>>>> error occurs.
>>>>  * If pkt is empty (i.e. NULL, or pkt->data is NULL and
>>>> pkt->side_data_elems zero),
>>>> @@ -4770,7 +4766,7 @@ int av_bsf_send_packet(AVBSFContext *ctx,
>>>> AVPacket *pkt);
>>>>  * an error occurs.
>>>>  *
>>>>  * @note one input packet may result in several output packets, so
>>>> after sending
>>>> - * a packet with av_bsf_send_packet(), this function needs to be
>>>> called
>>>> + * a packet with av_bsf_send_packet(), this function may need to be
>>>> called
>>>>  * repeatedly until it stops returning 0. It is also possible for a
>>>> filter to
>>>>  * output fewer packets than were sent to it, so this function may
>>>> return
>>>>  * AVERROR(EAGAIN) immediately after a successful av_bsf_send_packet()
>>>> call.
>>>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
>>>> index b9fc771a88..c79a8ebbdf 100644
>>>> --- a/libavcodec/bsf.c
>>>> +++ b/libavcodec/bsf.c
>>>> @@ -30,6 +30,7 @@
>>>>
>>>> struct AVBSFInternal {
>>>>     AVPacket *buffer_pkt;
>>>> +    AVPacket *out_pkt;
>>>>     int eof;
>>>> };
>>>>
>>>> @@ -46,8 +47,10 @@ void av_bsf_free(AVBSFContext **pctx)
>>>>     if (ctx->filter->priv_class && ctx->priv_data)
>>>>         av_opt_free(ctx->priv_data);
>>>>
>>>> -    if (ctx->internal)
>>>> +    if (ctx->internal) {
>>>>         av_packet_free(&ctx->internal->buffer_pkt);
>>>> +        av_packet_free(&ctx->internal->out_pkt);
>>>> +    }
>>>>     av_freep(&ctx->internal);
>>>>     av_freep(&ctx->priv_data);
>>>>
>>>> @@ -112,7 +115,8 @@ int av_bsf_alloc(const AVBitStreamFilter *filter,
>>>> AVBSFContext **pctx)
>>>>     ctx->internal = bsfi;
>>>>
>>>>     bsfi->buffer_pkt = av_packet_alloc();
>>>> -    if (!bsfi->buffer_pkt) {
>>>> +    bsfi->out_pkt = av_packet_alloc();
>>>> +    if (!bsfi->buffer_pkt || !bsfi->out_pkt) {
>>>>         ret = AVERROR(ENOMEM);
>>>>         goto fail;
>>>>     }
>>>> @@ -185,6 +189,7 @@ void av_bsf_flush(AVBSFContext *ctx)
>>>>     bsfi->eof = 0;
>>>>
>>>>     av_packet_unref(bsfi->buffer_pkt);
>>>> +    av_packet_unref(bsfi->out_pkt);
>>>>
>>>>     if (ctx->filter->flush)
>>>>         ctx->filter->flush(ctx);
>>>> @@ -205,10 +210,21 @@ int av_bsf_send_packet(AVBSFContext *ctx,
>>>> AVPacket *pkt)
>>>>         return AVERROR(EINVAL);
>>>>     }
>>>>
>>>> +    if (!bsfi->buffer_pkt->data &&
>>>> +        !bsfi->buffer_pkt->side_data_elems)
>>>> +        goto end;
>>>> +
>>>> +    if (!bsfi->out_pkt->data && !bsfi->out_pkt->side_data_elems) {
>>>> +        ret = ctx->filter->filter(ctx, bsfi->out_pkt);
>>>> +        if (ret < 0 && ret != AVERROR(EAGAIN) && ret != AVERROR_EOF)
>>>> +            return ret;
>>>> +    }
>>>> +
>>>>     if (bsfi->buffer_pkt->data ||
>>>>         bsfi->buffer_pkt->side_data_elems)
>>>>         return AVERROR(EAGAIN);
>>>>
>>>> +end:
>>>>     ret = av_packet_make_refcounted(pkt);
>>>>     if (ret < 0)
>>>>         return ret;
>>>> @@ -219,7 +235,17 @@ int av_bsf_send_packet(AVBSFContext *ctx,
>>>> AVPacket *pkt)
>>>>
>>>> int av_bsf_receive_packet(AVBSFContext *ctx, AVPacket *pkt)
>>>> {
>>>> -    return ctx->filter->filter(ctx, pkt);
>>>> +    AVBSFInternal *bsfi = ctx->internal;
>>>> +    int ret;
>>>> +
>>>> +    if (!bsfi->out_pkt->data && !bsfi->out_pkt->side_data_elems) {
>>>> +        ret = ctx->filter->filter(ctx, bsfi->out_pkt);
>>>> +        if (ret < 0)
>>>> +            return ret;
>>>> +    }
>>>> +    av_packet_move_ref(pkt, bsfi->out_pkt);
>>>> +
>>>> +    return 0;
>>>> }
>>>>
>>>> int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt)
>>>> @@ -227,12 +253,9 @@ int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket
>>>> **pkt)
>>>>     AVBSFInternal *bsfi = ctx->internal;
>>>>     AVPacket *tmp_pkt;
>>>>
>>>> -    if (bsfi->eof)
>>>> -        return AVERROR_EOF;
>>>> -
>>>>     if (!bsfi->buffer_pkt->data &&
>>>>         !bsfi->buffer_pkt->side_data_elems)
>>>> -        return AVERROR(EAGAIN);
>>>> +        return bsfi->eof ? AVERROR_EOF : AVERROR(EAGAIN);
>>>>
>>>>     tmp_pkt = av_packet_alloc();
>>>>     if (!tmp_pkt)
>>>> @@ -248,12 +271,9 @@ int ff_bsf_get_packet_ref(AVBSFContext *ctx,
>>>> AVPacket *pkt)
>>>> {
>>>>     AVBSFInternal *bsfi = ctx->internal;
>>>>
>>>> -    if (bsfi->eof)
>>>> -        return AVERROR_EOF;
>>>> -
>>>>     if (!bsfi->buffer_pkt->data &&
>>>>         !bsfi->buffer_pkt->side_data_elems)
>>>> -        return AVERROR(EAGAIN);
>>>> +        return bsfi->eof ? AVERROR_EOF : AVERROR(EAGAIN);
>>>>
>>>>     av_packet_move_ref(pkt, bsfi->buffer_pkt);
>>>>
>>>> -- 
>>>> 2.26.0
>>>>
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel@ffmpeg.org
>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>
>>>> To unsubscribe, visit link above, or email
>>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Marton Balint April 19, 2020, 7:32 p.m. UTC | #6
On Sun, 19 Apr 2020, James Almer wrote:

> On 4/19/2020 3:26 PM, Marton Balint wrote:
>> 
>> 
>> On Sun, 19 Apr 2020, James Almer wrote:
>> 
>>> On 4/19/2020 1:01 PM, Marton Balint wrote:
>>>>
>>>>
>>>> On Sun, 19 Apr 2020, James Almer wrote:
>>>>
>>>>> Process input data as soon as it's fed to av_bsf_send_packet(),
>>>>> instead of
>>>>> storing a single packet and expecting the user to call
>>>>> av_bsf_receive_packet()
>>>>> in order to trigger the decoding process before they are allowed to
>>>>> feed more
>>>>> data.
>>>>>
>>>>> This puts the bsf API more in line with the decoupled decode API,
>>>>> without
>>>>> breaking existing using it.
>>>>
>>>> Well, previously it was assumed that av_bsf_send_packet() is never
>>>> returning AVERROR_INVALIDDATA, that is no longer the case. That matters
>>>> because current code in mux.c ingnores av_bsf_receive_packet() errors
>>>> but not av_bsf_send_packet() errors.
>>>
>>> The doxy states av_bsf_send_packet() can return an error, even if up
>>> till now it hasn't.
>>>
>>> Also, as i stated in the addendum below the Signed-off line, current
>>> users following the recommended API usage (one call to
>>> av_bsf_send_packet(), then several to av_bsf_receive_packet()) will see
>>> no changes at all. So if you did that and expected no errors from
>>> av_bsf_send_packet() in your mux.c code, you'll still get none.
>>>
>>>>
>>>> Unless there is some benefit I am not seeing, I'd rather keep things as
>>>> is. Sorry.
>>>
>>> The benefit is relaxing the current constrains of the API for new users
>>> (including mux.c if you want to take advantage of it),
>> 
>> How is this making things easier for the API user? After the patch, the
>> framework can - in most cases - buffer 2 packets instead of 1. So what?
>> User app still have to implement that if it gets EAGAIN in send_packet,
>> it has to call receive_packet and vica versa. Unless it uses the
>> "recommended" practice, where it can assume success of send_packet for a
>> completetly drained bsf.
>
> The difference is that i can now call send_packet as many times as the
> underlying bsf lets me,

That is the main difference, yes, thanks.

> much like the decode send_packet, which allows
> me to build the loop in many different ways and not just the currently
> enforced "Send one packet, receive packets until drained" one.
>
>> 
>> You say that the API should be the same as for encode/decode. Well, it
>> already is.
>
> It certainly isn't. Unlike the decode API, I can't feed the
> av1_frame_merge bsf all the packets it needs before it has one ready for
> output. I need to feed them one, interleaved with at least one
> receive_packet call that will tell me that i need send more before it
> can output anything.

I just don't see why this is an issue? Does it measurably help performance 
if the number of API calls are reduced which return EAGAIN? Or does this 
have some other benefit?

>
>> Decode/encode API makes no guarantees about the number of
>> possibly buffered frames/packets. It only says that one of successive
>> send/receive calls must succeed and not return EAGAIN. And that is true
>> with the current BSF implementation.
>> 
>> You can remove the comment that "the filter must be completely drained",
>> or change it to that it is recommended to completetly drain the bsf. But
>> the code need no changes, it will still be API-compatible with the
>> decode API. It just uses internally a stricter scheme, and I like that
>> it does.
>
> I'm removing the stricter scheme without altering the workflow of users
> of said strict scheme. It's not breaking your mux.c code, right?

Right.

> Or at least it's not meant to if you followed the current strict 
> guidelines. So what exactly are you against of?

I am not against it anymore, feel free to go ahead. I am just still not 
sure about the general usefulness.

Thanks,
Marton
James Almer April 19, 2020, 7:42 p.m. UTC | #7
On 4/19/2020 4:32 PM, Marton Balint wrote:
> 
> 
> On Sun, 19 Apr 2020, James Almer wrote:
> 
>> On 4/19/2020 3:26 PM, Marton Balint wrote:
>>>
>>>
>>> On Sun, 19 Apr 2020, James Almer wrote:
>>>
>>>> On 4/19/2020 1:01 PM, Marton Balint wrote:
>>>>>
>>>>>
>>>>> On Sun, 19 Apr 2020, James Almer wrote:
>>>>>
>>>>>> Process input data as soon as it's fed to av_bsf_send_packet(),
>>>>>> instead of
>>>>>> storing a single packet and expecting the user to call
>>>>>> av_bsf_receive_packet()
>>>>>> in order to trigger the decoding process before they are allowed to
>>>>>> feed more
>>>>>> data.
>>>>>>
>>>>>> This puts the bsf API more in line with the decoupled decode API,
>>>>>> without
>>>>>> breaking existing using it.
>>>>>
>>>>> Well, previously it was assumed that av_bsf_send_packet() is never
>>>>> returning AVERROR_INVALIDDATA, that is no longer the case. That
>>>>> matters
>>>>> because current code in mux.c ingnores av_bsf_receive_packet() errors
>>>>> but not av_bsf_send_packet() errors.
>>>>
>>>> The doxy states av_bsf_send_packet() can return an error, even if up
>>>> till now it hasn't.
>>>>
>>>> Also, as i stated in the addendum below the Signed-off line, current
>>>> users following the recommended API usage (one call to
>>>> av_bsf_send_packet(), then several to av_bsf_receive_packet()) will see
>>>> no changes at all. So if you did that and expected no errors from
>>>> av_bsf_send_packet() in your mux.c code, you'll still get none.
>>>>
>>>>>
>>>>> Unless there is some benefit I am not seeing, I'd rather keep
>>>>> things as
>>>>> is. Sorry.
>>>>
>>>> The benefit is relaxing the current constrains of the API for new users
>>>> (including mux.c if you want to take advantage of it),
>>>
>>> How is this making things easier for the API user? After the patch, the
>>> framework can - in most cases - buffer 2 packets instead of 1. So what?
>>> User app still have to implement that if it gets EAGAIN in send_packet,
>>> it has to call receive_packet and vica versa. Unless it uses the
>>> "recommended" practice, where it can assume success of send_packet for a
>>> completetly drained bsf.
>>
>> The difference is that i can now call send_packet as many times as the
>> underlying bsf lets me,
> 
> That is the main difference, yes, thanks.
> 
>> much like the decode send_packet, which allows
>> me to build the loop in many different ways and not just the currently
>> enforced "Send one packet, receive packets until drained" one.
>>
>>>
>>> You say that the API should be the same as for encode/decode. Well, it
>>> already is.
>>
>> It certainly isn't. Unlike the decode API, I can't feed the
>> av1_frame_merge bsf all the packets it needs before it has one ready for
>> output. I need to feed them one, interleaved with at least one
>> receive_packet call that will tell me that i need send more before it
>> can output anything.
> 
> I just don't see why this is an issue? Does it measurably help
> performance if the number of API calls are reduced which return EAGAIN?
> Or does this have some other benefit?

More freedom for the API user to write their loops by removing a strict
constrain, and making all our decouple input/output APIs behave as
similar as possible (Having them behave exactly the same is impossible
without breaking the bsf API, making it return EOF instead of 0 on
subsequent flush packets). Just that.

> 
>>
>>> Decode/encode API makes no guarantees about the number of
>>> possibly buffered frames/packets. It only says that one of successive
>>> send/receive calls must succeed and not return EAGAIN. And that is true
>>> with the current BSF implementation.
>>>
>>> You can remove the comment that "the filter must be completely drained",
>>> or change it to that it is recommended to completetly drain the bsf. But
>>> the code need no changes, it will still be API-compatible with the
>>> decode API. It just uses internally a stricter scheme, and I like that
>>> it does.
>>
>> I'm removing the stricter scheme without altering the workflow of users
>> of said strict scheme. It's not breaking your mux.c code, right?
> 
> Right.
> 
>> Or at least it's not meant to if you followed the current strict
>> guidelines. So what exactly are you against of?
> 
> I am not against it anymore, feel free to go ahead. I am just still not
> sure about the general usefulness.

Will wait a bit before pushing in any case. Thanks.

> 
> Thanks,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Andreas Rheinhardt April 19, 2020, 7:59 p.m. UTC | #8
James Almer:
> Process input data as soon as it's fed to av_bsf_send_packet(), instead of
> storing a single packet and expecting the user to call av_bsf_receive_packet()
> in order to trigger the decoding process before they are allowed to feed more
> data.
> 
> This puts the bsf API more in line with the decoupled decode API, without
> breaking existing using it.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Now without creating a new reference for the input packet, and behaving the
> exact same as before for current users that were following the doxy right down
> to the letter.
> 
> Also didn't rename buffer_pkt to reduce the size of the diff and easily find
> the actual changes and additions.
> 
>  libavcodec/avcodec.h |  6 +-----
>  libavcodec/bsf.c     | 42 +++++++++++++++++++++++++++++++-----------
>  2 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index b79b025e53..af2a1b0e90 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -4735,10 +4735,6 @@ int av_bsf_init(AVBSFContext *ctx);
>  /**
>   * Submit a packet for filtering.
>   *
> - * After sending each packet, the filter must be completely drained by calling
> - * av_bsf_receive_packet() repeatedly until it returns AVERROR(EAGAIN) or
> - * AVERROR_EOF.
> - *
>   * @param pkt the packet to filter. The bitstream filter will take ownership of
>   * the packet and reset the contents of pkt. pkt is not touched if an error occurs.
>   * If pkt is empty (i.e. NULL, or pkt->data is NULL and pkt->side_data_elems zero),
> @@ -4770,7 +4766,7 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt);
>   * an error occurs.
>   *
>   * @note one input packet may result in several output packets, so after sending
> - * a packet with av_bsf_send_packet(), this function needs to be called
> + * a packet with av_bsf_send_packet(), this function may need to be called
>   * repeatedly until it stops returning 0. It is also possible for a filter to
>   * output fewer packets than were sent to it, so this function may return
>   * AVERROR(EAGAIN) immediately after a successful av_bsf_send_packet() call.
> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
> index b9fc771a88..c79a8ebbdf 100644
> --- a/libavcodec/bsf.c
> +++ b/libavcodec/bsf.c
> @@ -30,6 +30,7 @@
>  
>  struct AVBSFInternal {
>      AVPacket *buffer_pkt;
> +    AVPacket *out_pkt;
>      int eof;
>  };
>  
> @@ -46,8 +47,10 @@ void av_bsf_free(AVBSFContext **pctx)
>      if (ctx->filter->priv_class && ctx->priv_data)
>          av_opt_free(ctx->priv_data);
>  
> -    if (ctx->internal)
> +    if (ctx->internal) {
>          av_packet_free(&ctx->internal->buffer_pkt);
> +        av_packet_free(&ctx->internal->out_pkt);
> +    }
>      av_freep(&ctx->internal);
>      av_freep(&ctx->priv_data);
>  
> @@ -112,7 +115,8 @@ int av_bsf_alloc(const AVBitStreamFilter *filter, AVBSFContext **pctx)
>      ctx->internal = bsfi;
>  
>      bsfi->buffer_pkt = av_packet_alloc();
> -    if (!bsfi->buffer_pkt) {
> +    bsfi->out_pkt = av_packet_alloc();
> +    if (!bsfi->buffer_pkt || !bsfi->out_pkt) {
>          ret = AVERROR(ENOMEM);
>          goto fail;
>      }
> @@ -185,6 +189,7 @@ void av_bsf_flush(AVBSFContext *ctx)
>      bsfi->eof = 0;
>  
>      av_packet_unref(bsfi->buffer_pkt);
> +    av_packet_unref(bsfi->out_pkt);
>  
>      if (ctx->filter->flush)
>          ctx->filter->flush(ctx);
> @@ -205,10 +210,21 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt)
>          return AVERROR(EINVAL);
>      }
>  
> +    if (!bsfi->buffer_pkt->data &&
> +        !bsfi->buffer_pkt->side_data_elems)

I think that this file needs a macro for "packet is empty", especially
with a patch like yours, so I have just sent [1].

> +        goto end;
> +
> +    if (!bsfi->out_pkt->data && !bsfi->out_pkt->side_data_elems) {
> +        ret = ctx->filter->filter(ctx, bsfi->out_pkt);
> +        if (ret < 0 && ret != AVERROR(EAGAIN) && ret != AVERROR_EOF)
> +            return ret;
> +    }
> +
>      if (bsfi->buffer_pkt->data ||
>          bsfi->buffer_pkt->side_data_elems)
>          return AVERROR(EAGAIN);
>  
> +end:
>      ret = av_packet_make_refcounted(pkt);
>      if (ret < 0)
>          return ret;
> @@ -219,7 +235,17 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt)
>  
>  int av_bsf_receive_packet(AVBSFContext *ctx, AVPacket *pkt)
>  {
> -    return ctx->filter->filter(ctx, pkt);
> +    AVBSFInternal *bsfi = ctx->internal;
> +    int ret;
> +
> +    if (!bsfi->out_pkt->data && !bsfi->out_pkt->side_data_elems) {
> +        ret = ctx->filter->filter(ctx, bsfi->out_pkt);

The way you are doing this leads to an unnecessary av_packet_move_ref()
and an unnecessary check (you can just "return ctx->filter->filter(ctx,
pkt);"). Given that the doc states that the supplied pkt has to be clean
and given that the filters unref it on error if they have modified it,
the packet will be clean on error. If you worry that some padding bytes
might have been modified, then change the doc to "On failure, pkt will
be clean/blank on return" instead of adding an av_packet_move_ref().

> +        if (ret < 0)
> +            return ret;
> +    }
> +    av_packet_move_ref(pkt, bsfi->out_pkt);
> +
> +    return 0;
>  }
>  
>  int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt)
> @@ -227,12 +253,9 @@ int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt)
>      AVBSFInternal *bsfi = ctx->internal;
>      AVPacket *tmp_pkt;
>  
> -    if (bsfi->eof)
> -        return AVERROR_EOF;
> -
>      if (!bsfi->buffer_pkt->data &&
>          !bsfi->buffer_pkt->side_data_elems)
> -        return AVERROR(EAGAIN);
> +        return bsfi->eof ? AVERROR_EOF : AVERROR(EAGAIN);
>  
>      tmp_pkt = av_packet_alloc();
>      if (!tmp_pkt)
> @@ -248,12 +271,9 @@ int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket *pkt)
>  {
>      AVBSFInternal *bsfi = ctx->internal;
>  
> -    if (bsfi->eof)
> -        return AVERROR_EOF;
> -
>      if (!bsfi->buffer_pkt->data &&
>          !bsfi->buffer_pkt->side_data_elems)
> -        return AVERROR(EAGAIN);
> +        return bsfi->eof ? AVERROR_EOF : AVERROR(EAGAIN);
>  
>      av_packet_move_ref(pkt, bsfi->buffer_pkt);
>  
> 

- Andreas

[1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-April/261013.html
James Almer April 19, 2020, 8:10 p.m. UTC | #9
On 4/19/2020 4:59 PM, Andreas Rheinhardt wrote:
>>  int av_bsf_receive_packet(AVBSFContext *ctx, AVPacket *pkt)
>>  {
>> -    return ctx->filter->filter(ctx, pkt);
>> +    AVBSFInternal *bsfi = ctx->internal;
>> +    int ret;
>> +
>> +    if (!bsfi->out_pkt->data && !bsfi->out_pkt->side_data_elems) {
>> +        ret = ctx->filter->filter(ctx, bsfi->out_pkt);
> The way you are doing this leads to an unnecessary av_packet_move_ref()
> and an unnecessary check (you can just "return ctx->filter->filter(ctx,
> pkt);"). Given that the doc states that the supplied pkt has to be clean
> and given that the filters unref it on error if they have modified it,
> the packet will be clean on error. If you worry that some padding bytes
> might have been modified, then change the doc to "On failure, pkt will
> be clean/blank on return" instead of adding an av_packet_move_ref().

"Should" is not "must". And even if it was, the user could still
mistakenly pass an uninitialized packet that an underlying bsf could
wrongly try to utilize (Calling av_grow_packet() instead of
av_new_packet(), calling av_packet_add_side_data(), or any kind of usage
that could behave unpredictably when there's existing data). So to
ensure bsfs will get a clean packet, the generic code should take care
of it, and this is the easiest way without outright returning an error
if an non blank packet is provided.

A call to av_pkt_move_ref() are almost free, does not care what's in the
dst packet, and is worth it for the extra assurance that bsfs will only
deal with clean packets from now on.
diff mbox series

Patch

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index b79b025e53..af2a1b0e90 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -4735,10 +4735,6 @@  int av_bsf_init(AVBSFContext *ctx);
 /**
  * Submit a packet for filtering.
  *
- * After sending each packet, the filter must be completely drained by calling
- * av_bsf_receive_packet() repeatedly until it returns AVERROR(EAGAIN) or
- * AVERROR_EOF.
- *
  * @param pkt the packet to filter. The bitstream filter will take ownership of
  * the packet and reset the contents of pkt. pkt is not touched if an error occurs.
  * If pkt is empty (i.e. NULL, or pkt->data is NULL and pkt->side_data_elems zero),
@@ -4770,7 +4766,7 @@  int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt);
  * an error occurs.
  *
  * @note one input packet may result in several output packets, so after sending
- * a packet with av_bsf_send_packet(), this function needs to be called
+ * a packet with av_bsf_send_packet(), this function may need to be called
  * repeatedly until it stops returning 0. It is also possible for a filter to
  * output fewer packets than were sent to it, so this function may return
  * AVERROR(EAGAIN) immediately after a successful av_bsf_send_packet() call.
diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
index b9fc771a88..c79a8ebbdf 100644
--- a/libavcodec/bsf.c
+++ b/libavcodec/bsf.c
@@ -30,6 +30,7 @@ 
 
 struct AVBSFInternal {
     AVPacket *buffer_pkt;
+    AVPacket *out_pkt;
     int eof;
 };
 
@@ -46,8 +47,10 @@  void av_bsf_free(AVBSFContext **pctx)
     if (ctx->filter->priv_class && ctx->priv_data)
         av_opt_free(ctx->priv_data);
 
-    if (ctx->internal)
+    if (ctx->internal) {
         av_packet_free(&ctx->internal->buffer_pkt);
+        av_packet_free(&ctx->internal->out_pkt);
+    }
     av_freep(&ctx->internal);
     av_freep(&ctx->priv_data);
 
@@ -112,7 +115,8 @@  int av_bsf_alloc(const AVBitStreamFilter *filter, AVBSFContext **pctx)
     ctx->internal = bsfi;
 
     bsfi->buffer_pkt = av_packet_alloc();
-    if (!bsfi->buffer_pkt) {
+    bsfi->out_pkt = av_packet_alloc();
+    if (!bsfi->buffer_pkt || !bsfi->out_pkt) {
         ret = AVERROR(ENOMEM);
         goto fail;
     }
@@ -185,6 +189,7 @@  void av_bsf_flush(AVBSFContext *ctx)
     bsfi->eof = 0;
 
     av_packet_unref(bsfi->buffer_pkt);
+    av_packet_unref(bsfi->out_pkt);
 
     if (ctx->filter->flush)
         ctx->filter->flush(ctx);
@@ -205,10 +210,21 @@  int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt)
         return AVERROR(EINVAL);
     }
 
+    if (!bsfi->buffer_pkt->data &&
+        !bsfi->buffer_pkt->side_data_elems)
+        goto end;
+
+    if (!bsfi->out_pkt->data && !bsfi->out_pkt->side_data_elems) {
+        ret = ctx->filter->filter(ctx, bsfi->out_pkt);
+        if (ret < 0 && ret != AVERROR(EAGAIN) && ret != AVERROR_EOF)
+            return ret;
+    }
+
     if (bsfi->buffer_pkt->data ||
         bsfi->buffer_pkt->side_data_elems)
         return AVERROR(EAGAIN);
 
+end:
     ret = av_packet_make_refcounted(pkt);
     if (ret < 0)
         return ret;
@@ -219,7 +235,17 @@  int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt)
 
 int av_bsf_receive_packet(AVBSFContext *ctx, AVPacket *pkt)
 {
-    return ctx->filter->filter(ctx, pkt);
+    AVBSFInternal *bsfi = ctx->internal;
+    int ret;
+
+    if (!bsfi->out_pkt->data && !bsfi->out_pkt->side_data_elems) {
+        ret = ctx->filter->filter(ctx, bsfi->out_pkt);
+        if (ret < 0)
+            return ret;
+    }
+    av_packet_move_ref(pkt, bsfi->out_pkt);
+
+    return 0;
 }
 
 int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt)
@@ -227,12 +253,9 @@  int ff_bsf_get_packet(AVBSFContext *ctx, AVPacket **pkt)
     AVBSFInternal *bsfi = ctx->internal;
     AVPacket *tmp_pkt;
 
-    if (bsfi->eof)
-        return AVERROR_EOF;
-
     if (!bsfi->buffer_pkt->data &&
         !bsfi->buffer_pkt->side_data_elems)
-        return AVERROR(EAGAIN);
+        return bsfi->eof ? AVERROR_EOF : AVERROR(EAGAIN);
 
     tmp_pkt = av_packet_alloc();
     if (!tmp_pkt)
@@ -248,12 +271,9 @@  int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket *pkt)
 {
     AVBSFInternal *bsfi = ctx->internal;
 
-    if (bsfi->eof)
-        return AVERROR_EOF;
-
     if (!bsfi->buffer_pkt->data &&
         !bsfi->buffer_pkt->side_data_elems)
-        return AVERROR(EAGAIN);
+        return bsfi->eof ? AVERROR_EOF : AVERROR(EAGAIN);
 
     av_packet_move_ref(pkt, bsfi->buffer_pkt);