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