diff mbox series

[FFmpeg-devel,1/3] avcodec: add a parser flag to enable keyframe tagging heuristics

Message ID 20210714143401.890-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/3] avcodec: add a parser flag to enable keyframe tagging heuristics
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

James Almer July 14, 2021, 2:33 p.m. UTC
It will be used to allow parsers to be more liberal when tagging packets as
keyframes.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/avcodec.h | 1 +
 1 file changed, 1 insertion(+)

Comments

Michael Niedermayer July 15, 2021, 8:23 p.m. UTC | #1
On Wed, Jul 14, 2021 at 11:33:59AM -0300, James Almer wrote:
> It will be used to allow parsers to be more liberal when tagging packets as
> keyframes.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/avcodec.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 8b97895aeb..8e3d888266 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -2809,6 +2809,7 @@ typedef struct AVCodecParserContext {
>  #define PARSER_FLAG_ONCE                      0x0002
>  /// Set if the parser has a valid file offset
>  #define PARSER_FLAG_FETCHED_OFFSET            0x0004
> +#define PARSER_FLAG_USE_KEYFRAME_HEURISTICS   0x0008
>  #define PARSER_FLAG_USE_CODEC_TS              0x1000

This doesnt "feel" like the best solution to me

dont you think it would be better to export all information ?

the concept of a keyframe is a point at which decoding can begin
that really are at least 3 points

the point at which packets begin to be input into the decoder

the point at which the decoder is able to return some decoded
data which closely resembles the encoder input

and the point at which the decoder output matches 1:1 the output
of a decoder starting from frame 0



Thanks

[...]
James Almer July 15, 2021, 8:55 p.m. UTC | #2
On 7/15/2021 5:23 PM, Michael Niedermayer wrote:
> On Wed, Jul 14, 2021 at 11:33:59AM -0300, James Almer wrote:
>> It will be used to allow parsers to be more liberal when tagging packets as
>> keyframes.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavcodec/avcodec.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>> index 8b97895aeb..8e3d888266 100644
>> --- a/libavcodec/avcodec.h
>> +++ b/libavcodec/avcodec.h
>> @@ -2809,6 +2809,7 @@ typedef struct AVCodecParserContext {
>>   #define PARSER_FLAG_ONCE                      0x0002
>>   /// Set if the parser has a valid file offset
>>   #define PARSER_FLAG_FETCHED_OFFSET            0x0004
>> +#define PARSER_FLAG_USE_KEYFRAME_HEURISTICS   0x0008
>>   #define PARSER_FLAG_USE_CODEC_TS              0x1000
> 
> This doesnt "feel" like the best solution to me
> 
> dont you think it would be better to export all information ?

The AVParser API is going to be removed at some point for something 
better that works on packets rather than raw buffers, so ideally we 
should not expand it too much, and leave more complex implementations 
for later.

Adding a flag is the simplest way to fix this for now, until a proper 
rework is done.

> 
> the concept of a keyframe is a point at which decoding can begin
> that really are at least 3 points
> 
> the point at which packets begin to be input into the decoder
> 
> the point at which the decoder is able to return some decoded
> data which closely resembles the encoder input
> 
> and the point at which the decoder output matches 1:1 the output
> of a decoder starting from frame 0

All parsers save for h264 are currently only tagging packets containing 
a coded bitstream that, when decoded, it fully resets the decoding state 
and depends on no previously parsed data or state, which is what (most) 
muxers expect. This approach here is making the h264 do the same by 
default (in line with the decoder), to ensure some muxers don't wrongly 
mark certain packets as sync samples, while letting others remain 
liberal about it.

Ideally yes, we'd propagate more detailed information in some way, which 
then the mp4 muxer can use to build sample groups for h264, hevc and 
av1. But the existing AVParser API is not adequate for that.

> 
> 
> 
> Thanks
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>
Michael Niedermayer July 16, 2021, 4:55 p.m. UTC | #3
On Thu, Jul 15, 2021 at 05:55:51PM -0300, James Almer wrote:
> On 7/15/2021 5:23 PM, Michael Niedermayer wrote:
> > On Wed, Jul 14, 2021 at 11:33:59AM -0300, James Almer wrote:
> > > It will be used to allow parsers to be more liberal when tagging packets as
> > > keyframes.
> > > 
> > > Signed-off-by: James Almer <jamrial@gmail.com>
> > > ---
> > >   libavcodec/avcodec.h | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > index 8b97895aeb..8e3d888266 100644
> > > --- a/libavcodec/avcodec.h
> > > +++ b/libavcodec/avcodec.h
> > > @@ -2809,6 +2809,7 @@ typedef struct AVCodecParserContext {
> > >   #define PARSER_FLAG_ONCE                      0x0002
> > >   /// Set if the parser has a valid file offset
> > >   #define PARSER_FLAG_FETCHED_OFFSET            0x0004
> > > +#define PARSER_FLAG_USE_KEYFRAME_HEURISTICS   0x0008
> > >   #define PARSER_FLAG_USE_CODEC_TS              0x1000
> > 
> > This doesnt "feel" like the best solution to me
> > 
> > dont you think it would be better to export all information ?
> 
> The AVParser API is going to be removed at some point for something better
> that works on packets rather than raw buffers, so ideally we should not
> expand it too much, and leave more complex implementations for later.

Iam not sure i follow your logic here

do you suggest to block setting lets call it "advanced keyframe" related work
across the codebase ? (that includes muxers and so on ?
Iam not saying thats a good or bad idea. Just trying to understand

Because to me it seems if the awnser is no to the question above
then this is not just about "Adding a flag is the simplest way to fix this"
but its adding codec specific code in muxers probably to extract the exta
information, and iam not sure that at this point its not easier to have
the parser export this information

And when the parser API is then replaced by "something better" the interface
changes less. If this heuristic flag is added then i assume it needs to be
removed and reimplemented with the API change.

I very likely am missing something
But i certainly do not suggest or ask for more work to be done. Rather the
opposit, i was wondering if its not less work overall to export the
information properly ...

thx

[...]
James Almer July 16, 2021, 6:43 p.m. UTC | #4
On 7/16/2021 1:55 PM, Michael Niedermayer wrote:
> On Thu, Jul 15, 2021 at 05:55:51PM -0300, James Almer wrote:
>> On 7/15/2021 5:23 PM, Michael Niedermayer wrote:
>>> On Wed, Jul 14, 2021 at 11:33:59AM -0300, James Almer wrote:
>>>> It will be used to allow parsers to be more liberal when tagging packets as
>>>> keyframes.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>    libavcodec/avcodec.h | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>>> index 8b97895aeb..8e3d888266 100644
>>>> --- a/libavcodec/avcodec.h
>>>> +++ b/libavcodec/avcodec.h
>>>> @@ -2809,6 +2809,7 @@ typedef struct AVCodecParserContext {
>>>>    #define PARSER_FLAG_ONCE                      0x0002
>>>>    /// Set if the parser has a valid file offset
>>>>    #define PARSER_FLAG_FETCHED_OFFSET            0x0004
>>>> +#define PARSER_FLAG_USE_KEYFRAME_HEURISTICS   0x0008
>>>>    #define PARSER_FLAG_USE_CODEC_TS              0x1000
>>>
>>> This doesnt "feel" like the best solution to me
>>>
>>> dont you think it would be better to export all information ?
>>
>> The AVParser API is going to be removed at some point for something better
>> that works on packets rather than raw buffers, so ideally we should not
>> expand it too much, and leave more complex implementations for later.
> 
> Iam not sure i follow your logic here
> 
> do you suggest to block setting lets call it "advanced keyframe" related work
> across the codebase ? (that includes muxers and so on ?
> Iam not saying thats a good or bad idea. Just trying to understand

I'm saying to not spend time extending the AVCodecParserContext API to 
export even more information when it's going to be replaced. A single 
flag to stop one parser from tagging more than it needs to is enough to 
work around the problem at hand.

> 
> Because to me it seems if the awnser is no to the question above
> then this is not just about "Adding a flag is the simplest way to fix this"
> but its adding codec specific code in muxers probably to extract the exta
> information, and iam not sure that at this point its not easier to have
> the parser export this information

Inserting the h264 parser internally in mpegtsenc is needed because it's 
the generic demuxer code that may or may not use one to assemble packets 
or fill missing information. The muxer side has no interaction with it, 
it only receives packets from some source.
So you add a new field to AVCodecParserContext to signal these "advanced 
keyframes". How does that information reach the muxer? You're now having 
to define or expand more stuff for this purpose.

Inserting it in the muxer also has the added benefit of not depending on 
the demuxing side from having inserted one to begin with. For example, 
open an mp4 file for which the demuxer was able to create an 
AVIndexEntry array and therefore will not tell the generic code that it 
needs a parser. If you remux that file to mpegts, only Sync Samples (Of 
which there may be none) will have been tagged as keyframes by the 
demuxer (Which is expected) and not all these other packets with 
recovery points like mpegts apparently wants.
If the muxer internally inserts the parser, all of them can be marked, 
and a flag is enough for that.

> 
> And when the parser API is then replaced by "something better" the interface
> changes less. If this heuristic flag is added then i assume it needs to be
> removed and reimplemented with the API change.

This flag would be removed alongside the rest of AVCodecParserContext 
API, but the replacement would not need something like it since we would 
then look into ensuring it exports more detailed information for a given 
packet in a proper way, and not just setting a generic and poorly 
defined context-wide key_frame field.

This is just a stopgap solution, not something that needs to be 
translated into a new API. I mean, just look how all these flags are not 
even namespaced, or how the main function doesn't even return error 
codes. A replacement hardly requires to be a 1:1 translation.

> 
> I very likely am missing something
> But i certainly do not suggest or ask for more work to be done. Rather the
> opposit, i was wondering if its not less work overall to export the
> information properly ...

The replacement for AVCodecParserContext would, if i recall correctly 
when it was suggested, be having the work done by bitstream filters. 
Some muxers and demuxers are already using bsfs in some cases, so it 
would be built on top of that, and maybe automatically inserted by both 
muxers and demuxers, not just the latter.

So considering that muxer specific code is needed right now, I don't 
think adding more fields to AVCodecParserContext will mean less work now 
or even later.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>
Michael Niedermayer July 17, 2021, 3:39 p.m. UTC | #5
On Fri, Jul 16, 2021 at 03:43:39PM -0300, James Almer wrote:
> On 7/16/2021 1:55 PM, Michael Niedermayer wrote:
> > On Thu, Jul 15, 2021 at 05:55:51PM -0300, James Almer wrote:
> > > On 7/15/2021 5:23 PM, Michael Niedermayer wrote:
> > > > On Wed, Jul 14, 2021 at 11:33:59AM -0300, James Almer wrote:
> > > > > It will be used to allow parsers to be more liberal when tagging packets as
> > > > > keyframes.
> > > > > 
> > > > > Signed-off-by: James Almer <jamrial@gmail.com>
> > > > > ---
> > > > >    libavcodec/avcodec.h | 1 +
> > > > >    1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > > > index 8b97895aeb..8e3d888266 100644
> > > > > --- a/libavcodec/avcodec.h
> > > > > +++ b/libavcodec/avcodec.h
> > > > > @@ -2809,6 +2809,7 @@ typedef struct AVCodecParserContext {
> > > > >    #define PARSER_FLAG_ONCE                      0x0002
> > > > >    /// Set if the parser has a valid file offset
> > > > >    #define PARSER_FLAG_FETCHED_OFFSET            0x0004
> > > > > +#define PARSER_FLAG_USE_KEYFRAME_HEURISTICS   0x0008
> > > > >    #define PARSER_FLAG_USE_CODEC_TS              0x1000
> > > > 
> > > > This doesnt "feel" like the best solution to me
> > > > 
> > > > dont you think it would be better to export all information ?
> > > 
> > > The AVParser API is going to be removed at some point for something better
> > > that works on packets rather than raw buffers, so ideally we should not
> > > expand it too much, and leave more complex implementations for later.
> > 
> > Iam not sure i follow your logic here
> > 
> > do you suggest to block setting lets call it "advanced keyframe" related work
> > across the codebase ? (that includes muxers and so on ?
> > Iam not saying thats a good or bad idea. Just trying to understand
> 
> I'm saying to not spend time extending the AVCodecParserContext API to
> export even more information when it's going to be replaced. A single flag
> to stop one parser from tagging more than it needs to is enough to work
> around the problem at hand.
> 
> > 
> > Because to me it seems if the awnser is no to the question above
> > then this is not just about "Adding a flag is the simplest way to fix this"
> > but its adding codec specific code in muxers probably to extract the exta
> > information, and iam not sure that at this point its not easier to have
> > the parser export this information
> 
> Inserting the h264 parser internally in mpegtsenc is needed because it's the
> generic demuxer code that may or may not use one to assemble packets or fill
> missing information. The muxer side has no interaction with it, it only
> receives packets from some source.
> So you add a new field to AVCodecParserContext to signal these "advanced
> keyframes". How does that information reach the muxer? You're now having to
> define or expand more stuff for this purpose.
> 
> Inserting it in the muxer also has the added benefit of not depending on the
> demuxing side from having inserted one to begin with. For example, open an
> mp4 file for which the demuxer was able to create an AVIndexEntry array and
> therefore will not tell the generic code that it needs a parser. If you
> remux that file to mpegts, only Sync Samples (Of which there may be none)
> will have been tagged as keyframes by the demuxer (Which is expected) and
> not all these other packets with recovery points like mpegts apparently
> wants.
> If the muxer internally inserts the parser, all of them can be marked, and a
> flag is enough for that.
> 
> > 
> > And when the parser API is then replaced by "something better" the interface
> > changes less. If this heuristic flag is added then i assume it needs to be
> > removed and reimplemented with the API change.
> 
> This flag would be removed alongside the rest of AVCodecParserContext API,
> but the replacement would not need something like it since we would then
> look into ensuring it exports more detailed information for a given packet
> in a proper way, 
> and not just setting a generic and poorly defined
> context-wide key_frame field.
> 
> This is just a stopgap solution, not something that needs to be translated
> into a new API. I mean, just look how all these flags are not even
> namespaced, or how the main function doesn't even return error codes. A
> replacement hardly requires to be a 1:1 translation.
> 
> > 
> > I very likely am missing something
> > But i certainly do not suggest or ask for more work to be done. Rather the
> > opposit, i was wondering if its not less work overall to export the
> > information properly ...
> 
> The replacement for AVCodecParserContext would, if i recall correctly when
> it was suggested, be having the work done by bitstream filters. Some muxers
> and demuxers are already using bsfs in some cases, so it would be built on
> top of that, and maybe automatically inserted by both muxers and demuxers,
> not just the latter.
> 
> So considering that muxer specific code is needed right now, I don't think
> adding more fields to AVCodecParserContext will mean less work now or even
> later.

I see your point of view.

The way i saw it was
1. We at some point likely want to export "advanced keyframe" information from
   demuxers
2. We want this information to be presented to the user in a consistent
   way and that is needed no matter if Parsers or bsfs extract it when its
   not nativly set from demuxers
3. If we had such a data structure, setting this from the current Parser is
   very simple and much cleaner than this flag it also would make it available
   to the user applications and not just muxer internal.

What i suggested is probably less work overall. But it would reorder steps
and put alot of other work before this. That may be undesirable so your
"hack" might be just more convenient now and iam not against it.   

thx

[...]
Andreas Rheinhardt July 18, 2021, 1:23 a.m. UTC | #6
James Almer:
> On 7/15/2021 5:23 PM, Michael Niedermayer wrote:
>>
>> the concept of a keyframe is a point at which decoding can begin
>> that really are at least 3 points
>>
>> the point at which packets begin to be input into the decoder
>>
>> the point at which the decoder is able to return some decoded
>> data which closely resembles the encoder input
>>
>> and the point at which the decoder output matches 1:1 the output
>> of a decoder starting from frame 0
> 
> All parsers save for h264 are currently only tagging packets containing
> a coded bitstream that, when decoded, it fully resets the decoding state
> and depends on no previously parsed data or state, which is what (most)
> muxers expect. This approach here is making the h264 do the same by
> default (in line with the decoder), to ensure some muxers don't wrongly
> mark certain packets as sync samples, while letting others remain
> liberal about it.
> 
That is not true: The HEVC parser marks packets that may have leading
RASL pictures as keyframe; such frames are not sync samples according to
my version of ISO/IEC 14496-15. (Furthermore, for parsers that don't set
key_frame the recommended fallback is by checking pict_type for
AV_PICTURE_TYPE_I (parse_packet() in libavformat/utils.c does this); if
one follows this, then MPEG-2 I-frames will be marked as keyframes, even
when they are not sync samples in ISOBMFF if there is an open GOP.)

It seems to be mostly followed that random access points are keyframes
even if they are not IDR frames/even if there is an open GOP. In fact,
the AV1 parser (which does not set it for delayed random access points
(AV1's equivalent of open GOP)) seems to be an exception.

And your claim (also contained in the commit message) that this brings
the parser in line with the decoder is wrong, too: output_frame() in
h264dec.c sets key_frame depending on sei_recovery_frame_cnt.

- Andreas
James Almer July 18, 2021, 1:32 a.m. UTC | #7
On 7/17/2021 10:23 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 7/15/2021 5:23 PM, Michael Niedermayer wrote:
>>>
>>> the concept of a keyframe is a point at which decoding can begin
>>> that really are at least 3 points
>>>
>>> the point at which packets begin to be input into the decoder
>>>
>>> the point at which the decoder is able to return some decoded
>>> data which closely resembles the encoder input
>>>
>>> and the point at which the decoder output matches 1:1 the output
>>> of a decoder starting from frame 0
>>
>> All parsers save for h264 are currently only tagging packets containing
>> a coded bitstream that, when decoded, it fully resets the decoding state
>> and depends on no previously parsed data or state, which is what (most)
>> muxers expect. This approach here is making the h264 do the same by
>> default (in line with the decoder), to ensure some muxers don't wrongly
>> mark certain packets as sync samples, while letting others remain
>> liberal about it.
>>
> That is not true: The HEVC parser marks packets that may have leading
> RASL pictures as keyframe; such frames are not sync samples according to
> my version of ISO/IEC 14496-15. (Furthermore, for parsers that don't set
> key_frame the recommended fallback is by checking pict_type for
> AV_PICTURE_TYPE_I (parse_packet() in libavformat/utils.c does this); if
> one follows this, then MPEG-2 I-frames will be marked as keyframes, even
> when they are not sync samples in ISOBMFF if there is an open GOP.)
> 
> It seems to be mostly followed that random access points are keyframes
> even if they are not IDR frames/even if there is an open GOP. In fact,
> the AV1 parser (which does not set it for delayed random access points
> (AV1's equivalent of open GOP)) seems to be an exception.
> 
> And your claim (also contained in the commit message) that this brings
> the parser in line with the decoder is wrong, too: output_frame() in
> h264dec.c sets key_frame depending on sei_recovery_frame_cnt.

True, missed that. But for example in the case of intra_refresh.h264 it 
would not trigger, whereas the parser does tag it.

> 
> - Andreas
> _______________________________________________
> 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 July 18, 2021, 3:31 a.m. UTC | #8
James Almer:
> On 7/17/2021 10:23 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 7/15/2021 5:23 PM, Michael Niedermayer wrote:
>>>>
>>>> the concept of a keyframe is a point at which decoding can begin
>>>> that really are at least 3 points
>>>>
>>>> the point at which packets begin to be input into the decoder
>>>>
>>>> the point at which the decoder is able to return some decoded
>>>> data which closely resembles the encoder input
>>>>
>>>> and the point at which the decoder output matches 1:1 the output
>>>> of a decoder starting from frame 0
>>>
>>> All parsers save for h264 are currently only tagging packets containing
>>> a coded bitstream that, when decoded, it fully resets the decoding state
>>> and depends on no previously parsed data or state, which is what (most)
>>> muxers expect. This approach here is making the h264 do the same by
>>> default (in line with the decoder), to ensure some muxers don't wrongly
>>> mark certain packets as sync samples, while letting others remain
>>> liberal about it.
>>>
>> That is not true: The HEVC parser marks packets that may have leading
>> RASL pictures as keyframe; such frames are not sync samples according to
>> my version of ISO/IEC 14496-15. (Furthermore, for parsers that don't set
>> key_frame the recommended fallback is by checking pict_type for
>> AV_PICTURE_TYPE_I (parse_packet() in libavformat/utils.c does this); if
>> one follows this, then MPEG-2 I-frames will be marked as keyframes, even
>> when they are not sync samples in ISOBMFF if there is an open GOP.)
>>
>> It seems to be mostly followed that random access points are keyframes
>> even if they are not IDR frames/even if there is an open GOP. In fact,
>> the AV1 parser (which does not set it for delayed random access points
>> (AV1's equivalent of open GOP)) seems to be an exception.
>>
>> And your claim (also contained in the commit message) that this brings
>> the parser in line with the decoder is wrong, too: output_frame() in
>> h264dec.c sets key_frame depending on sei_recovery_frame_cnt.
> 
> True, missed that. But for example in the case of intra_refresh.h264 it
> would not trigger, whereas the parser does tag it.
> 
Yeah, we don't handle intra refresh well at all; and I am pretty sure
that quite a lot of people would not consider the packets containing the
recovery point SEI message a keyframe if recovery_frame_cnt is > 0. As
has been said in the earlier thread, there is no single correct
definition for keyframe.

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 8b97895aeb..8e3d888266 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -2809,6 +2809,7 @@  typedef struct AVCodecParserContext {
 #define PARSER_FLAG_ONCE                      0x0002
 /// Set if the parser has a valid file offset
 #define PARSER_FLAG_FETCHED_OFFSET            0x0004
+#define PARSER_FLAG_USE_KEYFRAME_HEURISTICS   0x0008
 #define PARSER_FLAG_USE_CODEC_TS              0x1000
 
     int64_t offset;      ///< byte offset from starting packet start