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

[Marton Balint]

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