| Message ID | 20180531232439.10858-1-timo.teras@iki.fi |
|---|---|
| State | Accepted |
| Commit | 12205d2c896b7edbc929d4886e7bfda4b53538e5 |
| Headers | show |
On Fri, Jun 01, 2018 at 02:24:39AM +0300, Timo Teräs wrote: > Find codec tag for attached images using appropriate list of > supported image formats. > > This fixes writing the cover image to m4v/m4a and other container > formats that do not allow these codecs as a track. > > Signed-off-by: Timo Teräs <timo.teras@iki.fi> > --- > This replaces the previous patch: > PATCH] [RFC] avformat/movenc: support covert images for ipod muxer > > It appears that query_codec() is used only to figure out which stream > to select as the default copy stream. Fixing it properly for movenc > attached pictures is difficult, as the stream disposition would need > to be passed through the call stack. Additionally, it would make sense > to rewrite the selection logic open_output_file() to not use APIC > special tag and be based on the input/output dispositions. > > So this just adds proper handling of the codecs in mov_init() phase > which fixes most of the usability issues. > > Please backport to 4.0 stable branch too. > > Changes in v2: > - map the codec to the actual covr image tag value and > use that when writing the cover image instead of doing > a new switch over the codec > > libavformat/movenc.c | 30 ++++++++++++------------------ > 1 file changed, 12 insertions(+), 18 deletions(-) this seems to break: ./ffmpeg -i GF9720Repeal20the20Eighth20with20Helen20Linehan.m4a -t 1 test.mov [mov @ 0x2f45140] Could not find tag for codec h264 in stream #0, codec not currently supported in container Could not write header for output file #0 (incorrect codec parameters ?): Invalid argument Error initializing output stream 0:1 -- input can be found here: https://guiltyfeminist.libsyn.com/97-repeal-the-eighth-with-helen-linehan [...]
On Sat, 2 Jun 2018 00:46:30 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Jun 01, 2018 at 02:24:39AM +0300, Timo Teräs wrote: > > Find codec tag for attached images using appropriate list of > > supported image formats. > > > > This fixes writing the cover image to m4v/m4a and other container > > formats that do not allow these codecs as a track. > > > > Signed-off-by: Timo Teräs <timo.teras@iki.fi> > > --- > > This replaces the previous patch: > > PATCH] [RFC] avformat/movenc: support covert images for ipod muxer > > > > It appears that query_codec() is used only to figure out which > > stream to select as the default copy stream. Fixing it properly for > > movenc attached pictures is difficult, as the stream disposition > > would need to be passed through the call stack. Additionally, it > > would make sense to rewrite the selection logic open_output_file() > > to not use APIC special tag and be based on the input/output > > dispositions. > > > > So this just adds proper handling of the codecs in mov_init() phase > > which fixes most of the usability issues. > > > > Please backport to 4.0 stable branch too. > > > > Changes in v2: > > - map the codec to the actual covr image tag value and > > use that when writing the cover image instead of doing > > a new switch over the codec > > > > libavformat/movenc.c | 30 ++++++++++++------------------ > > 1 file changed, 12 insertions(+), 18 deletions(-) > > this seems to break: > ./ffmpeg -i GF9720Repeal20the20Eighth20with20Helen20Linehan.m4a -t 1 > test.mov > > [mov @ 0x2f45140] Could not find tag for codec h264 in stream #0, > codec not currently supported in container Could not write header for > output file #0 (incorrect codec parameters ?): Invalid argument Error > initializing output stream 0:1 -- > > input can be found here: > https://guiltyfeminist.libsyn.com/97-repeal-the-eighth-with-helen-linehan This has been broken all time, I think. It'll work correct with the patch if you add "-c copy" to preserve the cover image codecs. Before the cover image patch (before 4.0 release), it would just incorrectly convert the first cover image to a video track in the .mov. Without this patch (the current 4.0 and git head) will silently drop the cover image which got converted to h264 (default codec of the container). I wonder what the right thing to do would be? a) not auto-map any attached_pic video streams b) make ffmpeg by default just copy (not transcode) attached_pic c) allow AVOutputFormat to communicate default codec based on disposition d) have movenc ignore/warn about cover images in incorrect format e) something else? Ideas? Timo
On Sat, 2 Jun 2018 02:15:29 +0300 Timo Teras <timo.teras@iki.fi> wrote: > On Sat, 2 Jun 2018 00:46:30 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > input can be found here: > > https://guiltyfeminist.libsyn.com/97-repeal-the-eighth-with-helen-linehan > > This has been broken all time, I think. It'll work correct with the > patch if you add "-c copy" to preserve the cover image codecs. Actually this file is more complicated. Seems theres: - video track which the chapter track refers, so they are in fact timed chapter marker images. mov demux gives them out as: AV_DISPOSITION_ATTACHED_PIC | AV_DISPOSITION_TIMED_THUMBNAILS - cover image in the metadata given out as AV_DISPOSITION_ATTACHED_PIC As the original cover image just checks for AV_DISPOSITION_ATTACHED_PIC bit, it broke this by treating the AV_DISPOSITION_ATTACHED_PIC | AV_DISPOSITION_TIMED_THUMBNAILS streams as cover image too. Seems closer scrutiny is needed to the disposition bits. I wonder if we should test ==AV_DISPOSITION_ATTACHED_PIC or check that specific set of bits match? Though, I believe with the problem with this input file go away if we treat attached_pic|timed_thumbnail combo as before. In fact, after testing, then the conversion will just turn it to regular video track, and without the -t 1 will FAIL for other reasons. This is how pre-4.0 works too with this input file. Basically we just don't support properly muxing the AV_DISPOSITION_TIMED_THUMBNAILS streams currently, and thus the input file in question is not really handled right at all. > I wonder what the right thing to do would be? > a) not auto-map any attached_pic video streams > b) make ffmpeg by default just copy (not transcode) attached_pic > c) allow AVOutputFormat to communicate default codec based on > disposition > d) have movenc ignore/warn about cover images in incorrect format > e) something else? This question is still valid. Timo
On Sat, Jun 02, 2018 at 03:35:33AM +0300, Timo Teras wrote: > On Sat, 2 Jun 2018 02:15:29 +0300 > Timo Teras <timo.teras@iki.fi> wrote: > > > On Sat, 2 Jun 2018 00:46:30 +0200 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > input can be found here: > > > https://guiltyfeminist.libsyn.com/97-repeal-the-eighth-with-helen-linehan > > > > This has been broken all time, I think. It'll work correct with the > > patch if you add "-c copy" to preserve the cover image codecs. > > Actually this file is more complicated. Seems theres: > - video track which the chapter track refers, so they are in fact > timed chapter marker images. mov demux gives them out as: > AV_DISPOSITION_ATTACHED_PIC | AV_DISPOSITION_TIMED_THUMBNAILS > - cover image in the metadata given out as > AV_DISPOSITION_ATTACHED_PIC > > As the original cover image just checks for AV_DISPOSITION_ATTACHED_PIC > bit, it broke this by treating the AV_DISPOSITION_ATTACHED_PIC | > AV_DISPOSITION_TIMED_THUMBNAILS streams as cover image too. Seems > closer scrutiny is needed to the disposition bits. > > I wonder if we should test ==AV_DISPOSITION_ATTACHED_PIC or check that > specific set of bits match? > > Though, I believe with the problem with this input file go away if we > treat attached_pic|timed_thumbnail combo as before. In fact, after > testing, then the conversion will just turn it to regular video track, > and without the -t 1 will FAIL for other reasons. This is how pre-4.0 > works too with this input file. > > Basically we just don't support properly muxing the > AV_DISPOSITION_TIMED_THUMBNAILS streams currently, and thus the input > file in question is not really handled right at all. > > > I wonder what the right thing to do would be? > > a) not auto-map any attached_pic video streams > > b) make ffmpeg by default just copy (not transcode) attached_pic > > c) allow AVOutputFormat to communicate default codec based on > > disposition > > d) have movenc ignore/warn about cover images in incorrect format > > e) something else? > > This question is still valid. well, ultimatly ffmpeg should act close to what a inteligent user expects. For example a simple ffmpeg -i inputfile outputfile should produce a outputfile that results in similar presentation as the input when played. In respect to AV_DISPOSITION_TIMED_THUMBNAILS, one may expect that concatenation of files each with a single AV_DISPOSITION_ATTACHED_PIC would be converted to AV_DISPOSITION_TIMED_THUMBNAILS. behavior should be logic, predictable, simple. A user should be able to predict what ffmpeg will do so she can adjust parameters without much trial and error. It may be good to minimize the amount of exceptions for how streams are handled. [...]
diff --git a/libavformat/movenc.c b/libavformat/movenc.c index 7e616e866f..4ba90135df 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -1557,10 +1557,20 @@ static int mov_get_codec_tag(AVFormatContext *s, MOVTrack *track) return tag; } +static const AVCodecTag codec_cover_image_tags[] = { + { AV_CODEC_ID_MJPEG, 0xD }, + { AV_CODEC_ID_PNG, 0xE }, + { AV_CODEC_ID_BMP, 0x1B }, + { AV_CODEC_ID_NONE, 0 }, +}; + static int mov_find_codec_tag(AVFormatContext *s, MOVTrack *track) { int tag; + if (track->st->disposition & AV_DISPOSITION_ATTACHED_PIC) + return ff_codec_get_tag(codec_cover_image_tags, track->par->codec_id); + if (track->mode == MODE_MP4 || track->mode == MODE_PSP) tag = track->par->codec_tag; else if (track->mode == MODE_ISM) @@ -3429,7 +3439,7 @@ static int mov_write_covr(AVIOContext *pb, AVFormatContext *s) { MOVMuxContext *mov = s->priv_data; int64_t pos = 0; - int i, type; + int i; for (i = 0; i < s->nb_streams; i++) { MOVTrack *trk = &mov->tracks[i]; @@ -3439,22 +3449,6 @@ static int mov_write_covr(AVIOContext *pb, AVFormatContext *s) trk->cover_image.size <= 0) continue; - switch (st->codecpar->codec_id) { - case AV_CODEC_ID_MJPEG: - type = 0xD; - break; - case AV_CODEC_ID_PNG: - type = 0xE; - break; - case AV_CODEC_ID_BMP: - type = 0x1B; - break; - default: - av_log(s, AV_LOG_ERROR, "unsupported codec_id (0x%x) for cover", - st->codecpar->codec_id); - continue; - } - if (!pos) { pos = avio_tell(pb); avio_wb32(pb, 0); @@ -3462,7 +3456,7 @@ static int mov_write_covr(AVIOContext *pb, AVFormatContext *s) } avio_wb32(pb, 16 + trk->cover_image.size); ffio_wfourcc(pb, "data"); - avio_wb32(pb, type); + avio_wb32(pb, trk->tag); avio_wb32(pb , 0); avio_write(pb, trk->cover_image.data, trk->cover_image.size); }
Find codec tag for attached images using appropriate list of supported image formats. This fixes writing the cover image to m4v/m4a and other container formats that do not allow these codecs as a track. Signed-off-by: Timo Teräs <timo.teras@iki.fi> --- This replaces the previous patch: PATCH] [RFC] avformat/movenc: support covert images for ipod muxer It appears that query_codec() is used only to figure out which stream to select as the default copy stream. Fixing it properly for movenc attached pictures is difficult, as the stream disposition would need to be passed through the call stack. Additionally, it would make sense to rewrite the selection logic open_output_file() to not use APIC special tag and be based on the input/output dispositions. So this just adds proper handling of the codecs in mov_init() phase which fixes most of the usability issues. Please backport to 4.0 stable branch too. Changes in v2: - map the codec to the actual covr image tag value and use that when writing the cover image instead of doing a new switch over the codec libavformat/movenc.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-)