On Sat, 27 Jan 2018 01:44:04 +0100 (CET)
Marton Balint <c...@passwd.hu> wrote:
> On Sat, 27 Jan 2018, wm4 wrote:
>
> > The AV_DISPOSITION_ATTACHED_PIC flag is meant only for exporting a
> > static picture (such as embedded cover art) as pseudo video track. The
> > requirement is that there is exactly 1 packet, and that normal demuxing
> > does not return it (otherwise avformat_queue_attached_pictures() would
> > add it a second time). It should never used on actual video tracks that
> > return packets when demuxing.
>
> 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.
> >
> > I considered keeping the current behavior if there is exactly 1 frame
> > (according to nb_index_entries), but that would require additional work
> > to make sure avformat_queue_attached_pictures() does not add it a second
> > time, so I didn't bother.
> >
> > Reverts part of 697400eac07c0614f6b9f2e7615563982dbcbe4a and fixes
> > regressions with certain API users with such mp4 files.
> > ---
> > libavformat/mov.c | 18 +-----------------
> > 1 file changed, 1 insertion(+), 17 deletions(-)
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index 22faecfc17..f74be03359 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -6335,23 +6335,7 @@ static void mov_read_chapters(AVFormatContext *s)
> > cur_pos = avio_tell(sc->pb);
> >
> > if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) {
> > - st->disposition |= AV_DISPOSITION_ATTACHED_PIC |
> > AV_DISPOSITION_TIMED_THUMBNAILS;
> > - if (st->nb_index_entries) {
> > - // Retrieve the first frame, if possible
> > - AVPacket pkt;
> > - AVIndexEntry *sample = &st->index_entries[0];
> > - if (avio_seek(sc->pb, sample->pos, SEEK_SET) !=
> > sample->pos) {
> > - av_log(s, AV_LOG_ERROR, "Failed to retrieve first
> > frame\n");
> > - goto finish;
> > - }
> > -
> > - if (av_get_packet(sc->pb, &pkt, sample->size) < 0)
> > - goto finish;
> > -
> > - st->attached_pic = pkt;
> > - st->attached_pic.stream_index = st->index;
> > - st->attached_pic.flags |= AV_PKT_FLAG_KEY;
> > - }
> > + st->disposition |= AV_DISPOSITION_TIMED_THUMBNAILS;
>
> 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?
> 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.
> 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.
I can fix the avformat.h description of AV_DISPOSITION_TIMED_THUMBNAILS.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
