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

[James Almer]

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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".