[FFmpeg-devel] [PATCH] mov: do not mark timed thumbnail tracks as attached picture

[wm4]

On Sat, 27 Jan 2018 18:07:53 +0100 (CET)
Marton Balint <cus at passwd.hu> wrote:

> On Sat, 27 Jan 2018, wm4 wrote:
> 
> >> Docs in avformat.h only says that AV_DISPOSITION_ATTACHED_PIC "usually" 
> >> contains one packet.  
> >
> > It also implies AVStream.attached_pic is set, which can be only one
> > picture.  
> 
> From the docs I'd assume that attached_pic contains 
> the _first_ packet of the stream if multiple packets are there for a 
> stream with ATTACHED_PIC flag set.
> 
> >> AV_DISPOSITION_TIMED_THUMBNAILS is only ever used with 
> >> AV_DISPOSITION_ATTACHED_PIC according to the docs in avformat.h.  
> >
> > Well then there can be only 1 timed thumbnail.
> >
> > As documented, this flag
> > has been nonsense, and not even libavformat itself respected it. As I
> > wrote in the commit message, the first packet is added twice, once
> > injected by utils.c, and then again returned by mov.c.
> >
> > How does this make any sense?  
> 
> Returning the packet twice is a bug then. We should fix that instead of 
> changing the semantics of flags.

Sure, but it's an example how all assumptions about ATTACHED_PICs got
broken. ffplay has the same problem btw.: it makes ATTACHED_PIC tracks
entirely static. It uses that flag just like my own code to distinguish
single picture pseudo video tracks from tracks that actually have
timestamped frames.

> >  
> >> Like it or not, that is how the flag was introduced, so I'd rather not 
> >> change it now. It made sense to introduce it this way, because checking 
> >> for the ATTACHED_PIC flag was used (for example in ffmpeg when using the 
> >> capital V stream speicifier) to search for ordinary video streams. By 
> >> using this flag for timed thumbnails as well, applications did not have to 
> >> check for an additional flag when searching for ordinary video streams.  
> >
> > This is also nonsense. ATTACHED_PIC is not meant for any stream
> > selection stuff that ffmpeg.c might do, it means that it's a cover art
> > picture imported from metadata. (To be honest that doesn't make sense
> > either, it's just a dumb hack that should never have existed in the
> > first place and that broke a lot of things when it was introduced.)
> >
> > If you want some flag that means "ordinary video stream", it should
> > probably be introduced separately, instead of abusing ATTACHED_PIC for
> > it.  
> 
> Well, my intent was to able to select ordinary video streams when I 
> introduced the "V" stream specifier, and that is how I tried to document 
> it. Any patch that breaks this behaviour is a regression from my point of 
> view.

Well, it was also a regression from my point of view.

> >  
> >> So I am against this patch, probably better to fix the regression in the 
> >> API user side, because it seems to me this is one of those cases where 
> >> something will regress no matter what we do, so it is better to not cause 
> >> new regressions and advise the API user to work around existing ones 
> >> according to the slightly changed semantics of the API.  
> >
> > It's already "regressed" because it's been in a permanent state of
> > being buggy and making no sense at all.  
> 
> Returning a packet twice is obviously wrong, other than that it's just 
> how it works now.
> 
> > I can fix the avformat.h description of AV_DISPOSITION_TIMED_THUMBNAILS.  
> 
> If you want to pursue this then
> - make the docs consistent with your changes
> - document the API change in APIChanges
> - make sure ffmpeg.c "V" stream specifier does not regress
> - ask some insight from the orignal patch author who introduced the
>    TIMED_THUMBNAILS flag, he might have his own reasons or comments worth
>    considering

He agreed to my patch.

anyway, I already added a crappy workaround for this to my code. But
I'll block any other overloaded uses of ATTACHED_PIC that might be
added in the future, because that'd mean me (and possibly other API
users) would regress again and would have to check another flag as
exception.