| Message ID | 20180329064513.19252-1-timo.teras@iki.fi |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Mar 29, 2018 at 09:45:13AM +0300, Timo Teräs wrote: > Fixes https://trac.ffmpeg.org/ticket/2798 > > This makes movenc handle AV_DISPOSITION_ATTACHED_PIC and write > the associated pictures in iTunes cover atom. This corresponds > to how 'mov' demuxer parses and exposes the cover images when > reading. > > Tested to produce valid stream with: > ffmpeg -i movie.mp4 -i thumb.jpg -disposition:v:1 attached_pic > -map 0 -map 1 -c copy out.mp4 > > The cover image is also copied corretly with: > ffmpeg -i movie.mp4 -map 0 -c copy out.mp4 > > AtomicParseley says that the attached_pic stream is properly > not visible in the main tracks of the file. Though, I am not > sure if there's any side-effects of having internal stream > without any packets. We may need to arm the mov muxer with > additional checks for streams without any packets. > > It may make sense to move the code in mov_write_packet() that > grabs the attached picture to generic code in mux.c. Seems there's > currently no other muxer supporting cover images than mp3 and > it seems to use special code to handle them. > > Signed-off-by: Timo Teräs <timo.teras@iki.fi> > Cc: Matthieu Bouron <matthieu.bouron@gmail.com> > --- > fftools/ffmpeg.c | 1 + > libavformat/movenc.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 68 insertions(+) [...] > @@ -3451,6 +3499,7 @@ static int mov_write_ilst_tag(AVIOContext *pb, MOVMuxContext *mov, > mov_write_int8_metadata (s, pb, "hdvd", "hd_video", 1); > mov_write_int8_metadata (s, pb, "pgap", "gapless_playback",1); > mov_write_int8_metadata (s, pb, "cpil", "compilation", 1); > + mov_write_covr(pb, s); > mov_write_trkn_tag(pb, mov, s, 0); // track number > mov_write_trkn_tag(pb, mov, s, 1); // disc number > mov_write_tmpo_tag(pb, s); > @@ -5480,6 +5529,24 @@ static int mov_write_packet(AVFormatContext *s, AVPacket *pkt) > if (!pkt) { > mov_flush_fragment(s, 1); > return 1; > + } if (s->streams[pkt->stream_index]->disposition & AV_DISPOSITION_ATTACHED_PIC) { The if() should be in the next line or it should be a else if it looks like someone forgot a "else" even though it makes no difference here. thanks [...]
Hi On Sat, 31 Mar 2018 02:13:14 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Mar 29, 2018 at 09:45:13AM +0300, Timo Teräs wrote: > > Fixes https://trac.ffmpeg.org/ticket/2798 > > > > This makes movenc handle AV_DISPOSITION_ATTACHED_PIC and write > > the associated pictures in iTunes cover atom. This corresponds > > to how 'mov' demuxer parses and exposes the cover images when > > reading. > [...] > > > @@ -5480,6 +5529,24 @@ static int mov_write_packet(AVFormatContext > > *s, AVPacket *pkt) if (!pkt) { > > mov_flush_fragment(s, 1); > > return 1; > > + } if (s->streams[pkt->stream_index]->disposition & > > AV_DISPOSITION_ATTACHED_PIC) { > > The if() should be in the next line or it should be a else if > it looks like someone forgot a "else" even though it makes no > difference here. Thanks. Will fix this, and have few other minor cleanups too. Will resend, but I have additional questions/notes too: I think I will audit all for (...; ... < mov->nb_streams; ...) loops to see if they need to skip these attachments. In most cases it's implicitly done because they don't get track->entry incremented now. However, there might be few off especially with empty_moov flag. Wondering also if all those "track->entry <= 0" checks should be made to static inline function mov_track_has_data() or similar? Any comments if the patch should be split e.g. adding the attached_pic flag on it's own? Or if the code in mov_write_packet() to stuff the received data to the buffers should be in generic code, or made a helper function? And finally, should the codec tag/type negotiation code somehow be updated to recognize attachment_pic and accept only the valid three image formats supported for now? If yes, how to do it? Thanks, Timo
On Sat, Mar 31, 2018 at 09:54:01AM +0300, Timo Teras wrote: > Hi > > On Sat, 31 Mar 2018 02:13:14 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Thu, Mar 29, 2018 at 09:45:13AM +0300, Timo Teräs wrote: > > > Fixes https://trac.ffmpeg.org/ticket/2798 > > > > > > This makes movenc handle AV_DISPOSITION_ATTACHED_PIC and write > > > the associated pictures in iTunes cover atom. This corresponds > > > to how 'mov' demuxer parses and exposes the cover images when > > > reading. > > [...] > > > > > @@ -5480,6 +5529,24 @@ static int mov_write_packet(AVFormatContext > > > *s, AVPacket *pkt) if (!pkt) { > > > mov_flush_fragment(s, 1); > > > return 1; > > > + } if (s->streams[pkt->stream_index]->disposition & > > > AV_DISPOSITION_ATTACHED_PIC) { > > > > The if() should be in the next line or it should be a else if > > it looks like someone forgot a "else" even though it makes no > > difference here. > > Thanks. Will fix this, and have few other minor cleanups too. Will > resend, but I have additional questions/notes too: > > I think I will audit all for (...; ... < mov->nb_streams; ...) loops > to see if they need to skip these attachments. In most cases it's > implicitly done because they don't get track->entry incremented now. > However, there might be few off especially with empty_moov flag. > > Wondering also if all those "track->entry <= 0" checks should be made > to static inline function mov_track_has_data() or similar? sounds like a good idea > > Any comments if the patch should be split e.g. adding the attached_pic > flag on it's own? yes, that certainly should be split into a seperate patch > > Or if the code in mov_write_packet() to stuff the received data to the > buffers should be in generic code, or made a helper function? If it can be shared it could be in common code. yes > > And finally, should the codec tag/type negotiation code somehow be > updated to recognize attachment_pic and accept only the valid three > image formats supported for now? If yes, how to do it? > > Thanks, > Timo > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Hi, This patch set adds support for writing iTunes cover images in mov muxer. Copying of the picture to attached_pic was not yet made common code. It is better to do this when additional users are introduced. Passes fate. valgrind says runs to add/copy cover images were without errors. Changes since first RFC patch: - split ffmpeg attached_pic definition to separate commit - added missed 'else' in mov_write_packet() per review comment - additional checks to ignore attached_pic streams as needed - mov_write_covr() simplified slightly - av_packet_ref() used instead of deprecated av_copy_packet() Timo Teräs (3): ffmpeg: allow setting attached_pic disposition avformat/movenc: add rtp_hinting_needed() helper function avformat/movenc: support writing iTunes cover image fftools/ffmpeg.c | 1 + libavformat/avformat.h | 2 +- libavformat/movenc.c | 105 +++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 87 insertions(+), 21 deletions(-)
diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c index 1b2e37b8d8..b6751d7faa 100644 --- a/fftools/ffmpeg.c +++ b/fftools/ffmpeg.c @@ -3562,6 +3562,7 @@ static int init_output_stream(OutputStream *ost, char *error, int error_len) { "hearing_impaired" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = AV_DISPOSITION_HEARING_IMPAIRED }, .unit = "flags" }, { "visual_impaired" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = AV_DISPOSITION_VISUAL_IMPAIRED }, .unit = "flags" }, { "clean_effects" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = AV_DISPOSITION_CLEAN_EFFECTS }, .unit = "flags" }, + { "attached_pic" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = AV_DISPOSITION_ATTACHED_PIC }, .unit = "flags" }, { "captions" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = AV_DISPOSITION_CAPTIONS }, .unit = "flags" }, { "descriptions" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = AV_DISPOSITION_DESCRIPTIONS }, .unit = "flags" }, { "dependent" , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = AV_DISPOSITION_DEPENDENT }, .unit = "flags" }, diff --git a/libavformat/movenc.c b/libavformat/movenc.c index 230aeb6a6d..5400b2747e 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -3414,6 +3414,54 @@ static int mov_write_int8_metadata(AVFormatContext *s, AVIOContext *pb, return size; } +static int mov_write_covr(AVIOContext *pb, AVFormatContext *s) +{ + int64_t pos = 0; + int i; + + for (i = 0; i < s->nb_streams; i++) { + AVStream *st = s->streams[i]; + enum AVCodecID codec_id = st->codecpar->codec_id; + int type; + + if (!(st->disposition & AV_DISPOSITION_ATTACHED_PIC)) + continue; + + switch (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 %s for cover, skipping", + av_fourcc2str(st->codecpar->codec_tag)); + continue; + } + + if (!pos) { + pos = avio_tell(pb); + avio_wb32(pb, 0); + ffio_wfourcc(pb, "covr"); + } + + avio_wb32(pb, 16 + st->attached_pic.size); + ffio_wfourcc(pb, "data"); + avio_wb32(pb, type); + avio_wb32(pb , 0); + avio_write(pb, st->attached_pic.data, st->attached_pic.size); + } + + if (pos) + return update_size(pb, pos); + + return 0; +} + /* iTunes meta data list */ static int mov_write_ilst_tag(AVIOContext *pb, MOVMuxContext *mov, AVFormatContext *s) @@ -3451,6 +3499,7 @@ static int mov_write_ilst_tag(AVIOContext *pb, MOVMuxContext *mov, mov_write_int8_metadata (s, pb, "hdvd", "hd_video", 1); mov_write_int8_metadata (s, pb, "pgap", "gapless_playback",1); mov_write_int8_metadata (s, pb, "cpil", "compilation", 1); + mov_write_covr(pb, s); mov_write_trkn_tag(pb, mov, s, 0); // track number mov_write_trkn_tag(pb, mov, s, 1); // disc number mov_write_tmpo_tag(pb, s); @@ -5480,6 +5529,24 @@ static int mov_write_packet(AVFormatContext *s, AVPacket *pkt) if (!pkt) { mov_flush_fragment(s, 1); return 1; + } if (s->streams[pkt->stream_index]->disposition & AV_DISPOSITION_ATTACHED_PIC) { + AVStream *st = s->streams[pkt->stream_index]; + int ret; + + if (st->nb_frames >= 1) { + /* warn only once */ + if (st->nb_frames == 1) + av_log(s, AV_LOG_WARNING, "Got more than one picture in stream %d," + " ignoring.\n", pkt->stream_index); + return 0; + } + + if ((ret = av_copy_packet(&st->attached_pic, pkt)) < 0) + return ret; + + st->attached_pic.stream_index = st->index; + st->attached_pic.flags |= AV_PKT_FLAG_KEY; + return 0; } else { int i; MOVMuxContext *mov = s->priv_data;
Fixes https://trac.ffmpeg.org/ticket/2798 This makes movenc handle AV_DISPOSITION_ATTACHED_PIC and write the associated pictures in iTunes cover atom. This corresponds to how 'mov' demuxer parses and exposes the cover images when reading. Tested to produce valid stream with: ffmpeg -i movie.mp4 -i thumb.jpg -disposition:v:1 attached_pic -map 0 -map 1 -c copy out.mp4 The cover image is also copied corretly with: ffmpeg -i movie.mp4 -map 0 -c copy out.mp4 AtomicParseley says that the attached_pic stream is properly not visible in the main tracks of the file. Though, I am not sure if there's any side-effects of having internal stream without any packets. We may need to arm the mov muxer with additional checks for streams without any packets. It may make sense to move the code in mov_write_packet() that grabs the attached picture to generic code in mux.c. Seems there's currently no other muxer supporting cover images than mp3 and it seems to use special code to handle them. Signed-off-by: Timo Teräs <timo.teras@iki.fi> Cc: Matthieu Bouron <matthieu.bouron@gmail.com> --- fftools/ffmpeg.c | 1 + libavformat/movenc.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+)