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

[Marton Balint]

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.

>
> 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.

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.

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.

Regards,
Marton