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

[Michael Niedermayer]

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

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20210717/c028f913/attachment.sig>