[FFmpeg-devel] [PATCH] avformat/flacenc: support writing attached pictures

[Timo Teras]

On Wed, 4 Apr 2018 02:30:34 -0300
James Almer <jamrial at gmail.com> wrote:

> On 4/4/2018 2:11 AM, Timo Teras wrote:
> > On Wed,  4 Apr 2018 01:30:54 -0300
> > James Almer <jamrial at gmail.com> wrote:
> > 
> >> From: Rodger Combs <rodger.combs at gmail.com>
> >>
> >> Signed-off-by: James Almer <jamrial at gmail.com>
> >> ---
> >> Now using the packet list API instead of duplicating the code
> >> locally.
> >>
> >>  libavformat/flacenc.c | 274
> >> +++++++++++++++++++++++++++++++++++++++++++------- 1 file changed,
> >> 238 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
> >> index b894f9ef61..99f4ce7bad 100644
> >> [snip]
> >> -        c->streaminfo = av_malloc(FLAC_STREAMINFO_SIZE);
> >> -        if (!c->streaminfo)
> >> -            return AVERROR(ENOMEM);
> >> -        memcpy(c->streaminfo, streaminfo, FLAC_STREAMINFO_SIZE);
> >> +        /* warn only once for each stream */
> >> +        if (s->streams[pkt->stream_index]->nb_frames == 1) {
> >> +            av_log(s, AV_LOG_WARNING, "Got more than one picture
> >> in stream %d,"
> >> +                   " ignoring.\n", pkt->stream_index);
> >> +        }
> >> +        if (!c->waiting_pics ||
> >> s->streams[pkt->stream_index]->nb_frames >= 1)
> >> +            return 0;
> >> +
> >> +        if (index > c->audio_stream_idx)
> >> +            index--;
> >> +
> >> +        if ((ret = av_packet_ref(&c->pics[index], pkt)) < 0)
> >> +            return ret;
> >> +        c->waiting_pics--;
> >> +
> >> +        /* flush the buffered audio packets */
> >> +        if (!c->waiting_pics &&
> >> +            (ret = flac_queue_flush(s)) < 0)
> >> +            return ret;
> >>      }
> >>  
> >> -    if (pkt->size)
> >> -        avio_write(s->pb, pkt->data, pkt->size);
> >>      return 0;
> >>  }
> >>  
> > 
> > I've submitted attached picture support to movenc just now. Instead
> > of defining separate pictures queue, I'm reusing the
> > AVStream.attached_pic to hold it. Would it make sense to share this
> > small piece of code in some utility function (and use also
> > AVStream.attached_pic here)?
> > 
> > See:
> > https://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/227708.html
> 
> What's being done in this patch is not a picture queue, but an audio
> packet queue until the first video frame (the picture to be attached)
> shows up, and then all the audio is flushed to the output.

Sorry for using word 'queue'. But to me you are adding "AVPacket *pics;"
which is practically "a queue" of one packet or just a packet store for
picture per stream. Pictures are stored there until all of them are
received. I do the same, I just store it in the existing
AVStream.attached_pic. So this is exactly the same functional thing.
Please see my patch: the first hunk, and second last hunk modifying
mov_write_packet().

My suggestion is to have helper function like
avformat_set_attached_picture() or similar that would reference given
AVPacket in AVStream.attached_pic and handle the warning logging and
discarding of extra packets. Would this work?

Timo