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

[Marton Balint]

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.

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

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

Thanks,
Marton