Re: [FFmpeg-devel] [PATCH] [RFC] avformat/movenc: support writing iTunes cover image

[Michael Niedermayer]

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

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] [RFC] avformat/movenc: support writing iTunes cover image

[Timo Teras]

Hi

On Sat, 31 Mar 2018 02:13:14 +0200
Michael Niedermayer  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] [RFC] avformat/movenc: support writing iTunes cover image

[Michael Niedermayer]

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 
> Cc: Matthieu Bouron 
> ---
>  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

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel