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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
