On Sat, 27 Jan 2018 18:07:53 +0100 (CET) Marton Balint <c...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
