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

[wm4]

On Sat, 27 Jan 2018 01:44:04 +0100 (CET)
Marton Balint <cus at 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.