[FFmpeg-devel] [PATCH] mov: do not mark timed thumbnail tracks as attached picture
Marton Balint
cus at passwd.hu
Sat Jan 27 19:07:53 EET 2018
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
More information about the ffmpeg-devel
mailing list