[FFmpeg-devel] [PATCH 5/7] lavf/flacenc: support writing attached pictures

[Rostislav Pehlivanov]

On 2 August 2017 at 15:30, James Almer <jamrial at gmail.com> wrote:

> On 8/2/2017 3:30 AM, Rodger Combs wrote:
> > ---
> >  libavformat/flacenc.c | 285 ++++++++++++++++++++++++++++++
> +++++++++++++-------
> >  1 file changed, 250 insertions(+), 35 deletions(-)
> >
> > diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
> > index b894f9e..9768b6a 100644
>
> [...]
>
> > @@ -166,23 +341,63 @@ static int flac_write_trailer(struct
> AVFormatContext *s)
> >  static int flac_write_packet(struct AVFormatContext *s, AVPacket *pkt)
> >  {
> >      FlacMuxerContext *c = s->priv_data;
> > -    uint8_t *streaminfo;
> > -    int streaminfo_size;
> > +    if (pkt->stream_index == c->audio_stream_idx) {
> > +        if (c->waiting_pics) {
> > +            /* buffer audio packets until we get all the pictures */
> > +            AVPacketList *pktl = av_mallocz(sizeof(*pktl));
> > +            int ret;
> > +            if (!pktl) {
> > +                ret = AVERROR(ENOMEM);
> > +oom:
> > +                if (s->error_recognition & AV_EF_EXPLODE) {
> > +                    av_free(pktl);
> > +                    return ret;
> > +                }
> > +                av_log(s, AV_LOG_ERROR, "Out of memory in packet queue;
> skipping attached pictures\n");
> > +                c->waiting_pics = 0;
> > +                if ((ret = flac_queue_flush(s)) < 0)
> > +                    return ret;
> > +                return flac_write_audio_packet(s, pkt);
> > +            }
> > +
> > +            ret = av_packet_ref(&pktl->pkt, pkt);
> > +            if (ret < 0) {
> > +                av_freep(&pktl);
> > +                goto oom;
> > +            }
> > +
> > +            if (c->queue_end)
> > +                c->queue_end->next = pktl;
> > +            else
> > +                c->queue = pktl;
> > +            c->queue_end = pktl;
> > +        } else {
> > +            return flac_write_audio_packet(s, pkt);
> > +        }
> > +    } else {
> > +        int ret, index = pkt->stream_index;
> >
> > -    /* check for updated streaminfo */
> > -    streaminfo = av_packet_get_side_data(pkt, AV_PKT_DATA_NEW_EXTRADATA,
> > -                                         &streaminfo_size);
> > -    if (streaminfo && streaminfo_size == FLAC_STREAMINFO_SIZE) {
> > -        av_freep(&c->streaminfo);
> > +        /* 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;
> >
> > -        c->streaminfo = av_malloc(FLAC_STREAMINFO_SIZE);
> > -        if (!c->streaminfo)
> > -            return AVERROR(ENOMEM);
> > -        memcpy(c->streaminfo, streaminfo, FLAC_STREAMINFO_SIZE);
> > +        if (index > c->audio_stream_idx)
> > +            index--;
> > +
> > +        if ((ret = av_copy_packet(&c->pics[index], pkt)) < 0)
>
> Shouldn't this be av_packet_ref()?
> And they should probably be unreferenced after being consumed in
> flac_finish_header(), much like the audio packets are in
> flac_queue_flush().
>
> Also, you don't seem to be freeing c->pics anywhere.
>
>
av_packet_copy does a ref if the data is refcounted so its okay.
c->pics is indeed not freed, as well as the pictures it refs

If you free the list of pics and the pics themselves during
flac_finish_header() the patch will LGTM