diff mbox series

[FFmpeg-devel,4/6] ffmpeg: don't skip packets before a keyframe was seen if a bsf with delay is used

Message ID 20220214224156.39862-4-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/6] ffmpeg: flush delayed frames in codec copy scenarios | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

James Almer Feb. 14, 2022, 10:41 p.m. UTC
A keyframe could be buffered in the bsf and not be output until more packets
had been fed to it.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 fftools/ffmpeg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Anton Khirnov Feb. 15, 2022, 11:41 a.m. UTC | #1
Quoting James Almer (2022-02-14 23:41:54)
> A keyframe could be buffered in the bsf and not be output until more packets
> had been fed to it.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  fftools/ffmpeg.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 6aa0986f02..48d9016b4c 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -2026,7 +2026,8 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
>      }
>  
>      if ((!ost->frame_number && !(pkt->flags & AV_PKT_FLAG_KEY)) &&
> -        !ost->copy_initial_nonkeyframes)
> +        !ost->copy_initial_nonkeyframes &&
> +        !(ost->bsf_ctx && ost->bsf_ctx->filter->capabilities & AV_BSF_CAP_DELAY))
>          return;

Wouldn't it be simpler to add an OutputStream field that tracks whether
we've seen a keyframe packet yet? No new API required.
James Almer Feb. 15, 2022, 11:48 a.m. UTC | #2
On 2/15/2022 8:41 AM, Anton Khirnov wrote:
> Quoting James Almer (2022-02-14 23:41:54)
>> A keyframe could be buffered in the bsf and not be output until more packets
>> had been fed to it.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   fftools/ffmpeg.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>> index 6aa0986f02..48d9016b4c 100644
>> --- a/fftools/ffmpeg.c
>> +++ b/fftools/ffmpeg.c
>> @@ -2026,7 +2026,8 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
>>       }
>>   
>>       if ((!ost->frame_number && !(pkt->flags & AV_PKT_FLAG_KEY)) &&
>> -        !ost->copy_initial_nonkeyframes)
>> +        !ost->copy_initial_nonkeyframes &&
>> +        !(ost->bsf_ctx && ost->bsf_ctx->filter->capabilities & AV_BSF_CAP_DELAY))
>>           return;
> 
> Wouldn't it be simpler to add an OutputStream field that tracks whether
> we've seen a keyframe packet yet? No new API required.

Probably. It would also only trigger when a keyframe was seen instead of 
unconditionally for all delay flagged bsfs.

I still think this new API is a good addition, either way. Only a 
handful of bsfs buffer packets and require the caller to flush them 
after sending NULL (av1_frame_merge, vp9_superframe, and setts after 
this set) so library users could have all this time never signaled EOF 
and never noticed anything wrong, much like it happened here.
The presence of this flag might help library users know they really need 
to signal EOF.
Anton Khirnov Feb. 15, 2022, 12:03 p.m. UTC | #3
Quoting James Almer (2022-02-15 12:48:09)
> 
> 
> On 2/15/2022 8:41 AM, Anton Khirnov wrote:
> > Quoting James Almer (2022-02-14 23:41:54)
> >> A keyframe could be buffered in the bsf and not be output until more packets
> >> had been fed to it.
> >>
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> >> ---
> >>   fftools/ffmpeg.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> >> index 6aa0986f02..48d9016b4c 100644
> >> --- a/fftools/ffmpeg.c
> >> +++ b/fftools/ffmpeg.c
> >> @@ -2026,7 +2026,8 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
> >>       }
> >>   
> >>       if ((!ost->frame_number && !(pkt->flags & AV_PKT_FLAG_KEY)) &&
> >> -        !ost->copy_initial_nonkeyframes)
> >> +        !ost->copy_initial_nonkeyframes &&
> >> +        !(ost->bsf_ctx && ost->bsf_ctx->filter->capabilities & AV_BSF_CAP_DELAY))
> >>           return;
> > 
> > Wouldn't it be simpler to add an OutputStream field that tracks whether
> > we've seen a keyframe packet yet? No new API required.
> 
> Probably. It would also only trigger when a keyframe was seen instead of 
> unconditionally for all delay flagged bsfs.
> 
> I still think this new API is a good addition, either way. Only a 
> handful of bsfs buffer packets and require the caller to flush them 
> after sending NULL (av1_frame_merge, vp9_superframe, and setts after 
> this set) so library users could have all this time never signaled EOF 
> and never noticed anything wrong, much like it happened here.
> The presence of this flag might help library users know they really need 
> to signal EOF.

I don't see where the advantage would be. The callers still need to have
the flushing code, so might as well always call it.

The disadvantage for us is more complixity and we have to maintain the
list of delay BSFs.
James Almer Feb. 15, 2022, 12:12 p.m. UTC | #4
On 2/15/2022 9:03 AM, Anton Khirnov wrote:
> Quoting James Almer (2022-02-15 12:48:09)
>>
>>
>> On 2/15/2022 8:41 AM, Anton Khirnov wrote:
>>> Quoting James Almer (2022-02-14 23:41:54)
>>>> A keyframe could be buffered in the bsf and not be output until more packets
>>>> had been fed to it.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>    fftools/ffmpeg.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>>>> index 6aa0986f02..48d9016b4c 100644
>>>> --- a/fftools/ffmpeg.c
>>>> +++ b/fftools/ffmpeg.c
>>>> @@ -2026,7 +2026,8 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
>>>>        }
>>>>    
>>>>        if ((!ost->frame_number && !(pkt->flags & AV_PKT_FLAG_KEY)) &&
>>>> -        !ost->copy_initial_nonkeyframes)
>>>> +        !ost->copy_initial_nonkeyframes &&
>>>> +        !(ost->bsf_ctx && ost->bsf_ctx->filter->capabilities & AV_BSF_CAP_DELAY))
>>>>            return;
>>>
>>> Wouldn't it be simpler to add an OutputStream field that tracks whether
>>> we've seen a keyframe packet yet? No new API required.
>>
>> Probably. It would also only trigger when a keyframe was seen instead of
>> unconditionally for all delay flagged bsfs.
>>
>> I still think this new API is a good addition, either way. Only a
>> handful of bsfs buffer packets and require the caller to flush them
>> after sending NULL (av1_frame_merge, vp9_superframe, and setts after
>> this set) so library users could have all this time never signaled EOF
>> and never noticed anything wrong, much like it happened here.
>> The presence of this flag might help library users know they really need
>> to signal EOF.
> 
> I don't see where the advantage would be. The callers still need to have
> the flushing code, so might as well always call it.

Then we probably need to enforce it in the doxy, or at least strongly 
suggest it with a @note or @warning line to ensure you get complete output.
Right now it's optional, mentioned as "If you send a NULL packet, it 
will trigger EOF", meaning not doing so is still a valid scenario, and 
there's nothing letting the user know he's got packets stuck in the bsf 
even after receive_packet() returned EAGAIN if they don't.

> 
> The disadvantage for us is more complixity and we have to maintain the
> list of delay BSFs.
>
Anton Khirnov Feb. 21, 2022, 3:34 p.m. UTC | #5
Quoting James Almer (2022-02-15 13:12:51)
> On 2/15/2022 9:03 AM, Anton Khirnov wrote:
> > Quoting James Almer (2022-02-15 12:48:09)
> >>
> >>
> >> On 2/15/2022 8:41 AM, Anton Khirnov wrote:
> >>> Quoting James Almer (2022-02-14 23:41:54)
> >>>> A keyframe could be buffered in the bsf and not be output until more packets
> >>>> had been fed to it.
> >>>>
> >>>> Signed-off-by: James Almer <jamrial@gmail.com>
> >>>> ---
> >>>>    fftools/ffmpeg.c | 3 ++-
> >>>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> >>>> index 6aa0986f02..48d9016b4c 100644
> >>>> --- a/fftools/ffmpeg.c
> >>>> +++ b/fftools/ffmpeg.c
> >>>> @@ -2026,7 +2026,8 @@ static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
> >>>>        }
> >>>>    
> >>>>        if ((!ost->frame_number && !(pkt->flags & AV_PKT_FLAG_KEY)) &&
> >>>> -        !ost->copy_initial_nonkeyframes)
> >>>> +        !ost->copy_initial_nonkeyframes &&
> >>>> +        !(ost->bsf_ctx && ost->bsf_ctx->filter->capabilities & AV_BSF_CAP_DELAY))
> >>>>            return;
> >>>
> >>> Wouldn't it be simpler to add an OutputStream field that tracks whether
> >>> we've seen a keyframe packet yet? No new API required.
> >>
> >> Probably. It would also only trigger when a keyframe was seen instead of
> >> unconditionally for all delay flagged bsfs.
> >>
> >> I still think this new API is a good addition, either way. Only a
> >> handful of bsfs buffer packets and require the caller to flush them
> >> after sending NULL (av1_frame_merge, vp9_superframe, and setts after
> >> this set) so library users could have all this time never signaled EOF
> >> and never noticed anything wrong, much like it happened here.
> >> The presence of this flag might help library users know they really need
> >> to signal EOF.
> > 
> > I don't see where the advantage would be. The callers still need to have
> > the flushing code, so might as well always call it.
> 
> Then we probably need to enforce it in the doxy, or at least strongly 
> suggest it with a @note or @warning line to ensure you get complete output.
> Right now it's optional, mentioned as "If you send a NULL packet, it 
> will trigger EOF", meaning not doing so is still a valid scenario, and 
> there's nothing letting the user know he's got packets stuck in the bsf 
> even after receive_packet() returned EAGAIN if they don't.

The doxy for av_bsf_send_packet() already says:
If pkt is empty, it signals the end of the stream and will cause the
filter to output any packets it may have buffered internally.

But I can write something more explicit.
diff mbox series

Patch

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 6aa0986f02..48d9016b4c 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -2026,7 +2026,8 @@  static void do_streamcopy(InputStream *ist, OutputStream *ost, const AVPacket *p
     }
 
     if ((!ost->frame_number && !(pkt->flags & AV_PKT_FLAG_KEY)) &&
-        !ost->copy_initial_nonkeyframes)
+        !ost->copy_initial_nonkeyframes &&
+        !(ost->bsf_ctx && ost->bsf_ctx->filter->capabilities & AV_BSF_CAP_DELAY))
         return;
 
     if (!ost->frame_number && !ost->copy_prior_start) {