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

[James Almer]

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 at 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 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".
>