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

Submitted by James Almer on April 4, 2018, 4:30 a.m.

Details

Message ID 20180404043054.9696-1-jamrial@gmail.com
State Superseded
Headers show

Commit Message

James Almer April 4, 2018, 4:30 a.m.
From: Rodger Combs <rodger.combs@gmail.com>

Signed-off-by: James Almer <jamrial@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(-)

Comments

Timo Teräs April 4, 2018, 5:11 a.m.
On Wed,  4 Apr 2018 01:30:54 -0300
James Almer <jamrial@gmail.com> wrote:

> From: Rodger Combs <rodger.combs@gmail.com>
> 
> Signed-off-by: James Almer <jamrial@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
James Almer April 4, 2018, 5:30 a.m.
On 4/4/2018 2:11 AM, Timo Teras wrote:
> On Wed,  4 Apr 2018 01:30:54 -0300
> James Almer <jamrial@gmail.com> wrote:
> 
>> From: Rodger Combs <rodger.combs@gmail.com>
>>
>> Signed-off-by: James Almer <jamrial@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.

(Re-sending to the mailing list now, since my email client is kinda
stupid with its reply button in some situations)
Timo Teräs April 4, 2018, 5:40 a.m.
On Wed, 4 Apr 2018 02:30:34 -0300
James Almer <jamrial@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@gmail.com> wrote:
> > 
> >> From: Rodger Combs <rodger.combs@gmail.com>
> >>
> >> Signed-off-by: James Almer <jamrial@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

Patch hide | download patch | download mbox

diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
index b894f9ef61..99f4ce7bad 100644
--- a/libavformat/flacenc.c
+++ b/libavformat/flacenc.c
@@ -21,10 +21,13 @@ 
 
 #include "libavutil/channel_layout.h"
 #include "libavutil/opt.h"
+#include "libavutil/pixdesc.h"
 #include "libavcodec/flac.h"
 #include "avformat.h"
 #include "avio_internal.h"
 #include "flacenc.h"
+#include "id3v2.h"
+#include "internal.h"
 #include "vorbiscomment.h"
 #include "libavcodec/bytestream.h"
 
@@ -33,8 +36,16 @@  typedef struct FlacMuxerContext {
     const AVClass *class;
     int write_header;
 
+    int audio_stream_idx;
+    AVPacket *pics;
+    int nb_pics, waiting_pics;
+    /* audio packets are queued here until we get all the attached pictures */
+    AVPacketList *queue, *queue_end;
+
     /* updated streaminfo sent by the encoder at the end */
     uint8_t *streaminfo;
+
+    unsigned attached_types;
 } FlacMuxerContext;
 
 static int flac_write_block_padding(AVIOContext *pb, unsigned int n_padding_bytes,
@@ -74,31 +85,157 @@  static int flac_write_block_comment(AVIOContext *pb, AVDictionary **m,
     return 0;
 }
 
-static int flac_write_header(struct AVFormatContext *s)
+static int flac_write_picture(struct AVFormatContext *s, AVPacket *pkt)
 {
-    int ret;
-    int padding = s->metadata_header_padding;
-    AVCodecParameters *par = s->streams[0]->codecpar;
-    FlacMuxerContext *c   = s->priv_data;
-
-    if (!c->write_header)
+    FlacMuxerContext *c = s->priv_data;
+    AVIOContext *pb = s->pb;
+    const AVPixFmtDescriptor *pixdesc;
+    const CodecMime *mime = ff_id3v2_mime_tags;
+    AVDictionaryEntry *e;
+    const char *mimetype = NULL, *desc = "";
+    const AVStream *st = s->streams[pkt->stream_index];
+    int i, mimelen, desclen, type = 0;
+
+    if (!pkt->data)
         return 0;
 
-    if (s->nb_streams > 1) {
-        av_log(s, AV_LOG_ERROR, "only one stream is supported\n");
+    while (mime->id != AV_CODEC_ID_NONE) {
+        if (mime->id == st->codecpar->codec_id) {
+            mimetype = mime->str;
+            break;
+        }
+        mime++;
+    }
+    if (!mimetype) {
+        av_log(s, AV_LOG_ERROR, "No mimetype is known for stream %d, cannot "
+               "write an attached picture.\n", st->index);
+        return AVERROR(EINVAL);
+    }
+    mimelen = strlen(mimetype);
+
+    /* get the picture type */
+    e = av_dict_get(st->metadata, "comment", NULL, 0);
+    for (i = 0; e && i < FF_ARRAY_ELEMS(ff_id3v2_picture_types); i++) {
+        if (!av_strcasecmp(e->value, ff_id3v2_picture_types[i])) {
+            type = i;
+            break;
+        }
+    }
+
+    if ((c->attached_types & (1 << type)) & 0x6) {
+        av_log(s, AV_LOG_ERROR, "Duplicate attachment for type '%s'\n", ff_id3v2_picture_types[type]);
         return AVERROR(EINVAL);
     }
-    if (par->codec_id != AV_CODEC_ID_FLAC) {
-        av_log(s, AV_LOG_ERROR, "unsupported codec\n");
+
+    if (type == 1 && (st->codecpar->codec_id != AV_CODEC_ID_PNG ||
+                      st->codecpar->width != 32 ||
+                      st->codecpar->height != 32)) {
+        av_log(s, AV_LOG_ERROR, "File icon attachment must be a 32x32 PNG");
         return AVERROR(EINVAL);
     }
 
+    c->attached_types |= (1 << type);
+
+    /* get the description */
+    if ((e = av_dict_get(st->metadata, "title", NULL, 0)))
+        desc = e->value;
+    desclen = strlen(desc);
+
+    avio_w8(pb, 0x06);
+    avio_wb24(pb, 4 + 4 + mimelen + 4 + desclen + 4 + 4 + 4 + 4 + 4 + pkt->size);
+
+    avio_wb32(pb, type);
+
+    avio_wb32(pb, mimelen);
+    avio_write(pb, mimetype, mimelen);
+
+    avio_wb32(pb, desclen);
+    avio_write(pb, desc, desclen);
+
+    avio_wb32(pb, st->codecpar->width);
+    avio_wb32(pb, st->codecpar->height);
+    if ((pixdesc = av_pix_fmt_desc_get(st->codecpar->format)))
+        avio_wb32(pb, av_get_bits_per_pixel(pixdesc));
+    else
+        avio_wb32(pb, 0);
+    avio_wb32(pb, 0);
+
+    avio_wb32(pb, pkt->size);
+    avio_write(pb, pkt->data, pkt->size);
+    return 0;
+}
+
+static int flac_finish_header(struct AVFormatContext *s)
+{
+    FlacMuxerContext *c = s->priv_data;
+    int i, ret, padding = s->metadata_header_padding;
     if (padding < 0)
         padding = 8192;
     /* The FLAC specification states that 24 bits are used to represent the
      * size of a metadata block so we must clip this value to 2^24-1. */
     padding = av_clip_uintp2(padding, 24);
 
+    for (i = 0; i < c->nb_pics; i++) {
+        ret = flac_write_picture(s, &c->pics[i]);
+        av_packet_unref(&c->pics[i]);
+        if (ret)
+            return ret;
+    }
+
+    ret = flac_write_block_comment(s->pb, &s->metadata, !padding,
+                                   s->flags & AVFMT_FLAG_BITEXACT);
+    if (ret)
+        return ret;
+
+    /* The command line flac encoder defaults to placing a seekpoint
+     * every 10s.  So one might add padding to allow that later
+     * but there seems to be no simple way to get the duration here.
+     * So just add the amount requested by the user. */
+    if (padding)
+        flac_write_block_padding(s->pb, padding, 1);
+
+    return 0;
+}
+
+static int flac_write_header(struct AVFormatContext *s)
+{
+    AVCodecParameters *par;
+    FlacMuxerContext *c = s->priv_data;
+    int ret, i;
+
+    c->audio_stream_idx = -1;
+    for (i = 0; i < s->nb_streams; i++) {
+        AVStream *st = s->streams[i];
+        if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) {
+            if (c->audio_stream_idx >= 0 || st->codecpar->codec_id != AV_CODEC_ID_FLAC) {
+                av_log(s, AV_LOG_ERROR, "Invalid audio stream. Exactly one FLAC "
+                       "audio stream is required.\n");
+                return AVERROR(EINVAL);
+            }
+            par = s->streams[i]->codecpar;
+            c->audio_stream_idx = i;
+        } else if (st->codecpar->codec_type != AVMEDIA_TYPE_VIDEO) {
+            av_log(s, AV_LOG_ERROR, "Only audio streams and pictures are allowed in FLAC.\n");
+            return AVERROR(EINVAL);
+        } else if (st->codecpar->codec_id == AV_CODEC_ID_GIF) {
+            av_log(s, AV_LOG_ERROR, "GIF image support is not implemented.\n");
+            return AVERROR_PATCHWELCOME;
+        } else if (!c->write_header) {
+            av_log(s, AV_LOG_ERROR, "Can't write attached pictures without a header.\n");
+            return AVERROR(EINVAL);
+        }
+    }
+    if (c->audio_stream_idx < 0) {
+        av_log(s, AV_LOG_ERROR, "No audio stream present.\n");
+        return AVERROR(EINVAL);
+    }
+    c->waiting_pics = c->nb_pics = s->nb_streams - 1;
+    if (c->nb_pics && !(c->pics = av_calloc(c->nb_pics, sizeof(AVPacket))))
+        return AVERROR(ENOMEM);
+
+    if (!c->write_header)
+        return 0;
+
     ret = ff_flac_write_header(s->pb, par->extradata,
                                par->extradata_size, 0);
     if (ret)
@@ -121,18 +258,49 @@  static int flac_write_header(struct AVFormatContext *s)
         }
     }
 
-    ret = flac_write_block_comment(s->pb, &s->metadata, !padding,
-                                   s->flags & AVFMT_FLAG_BITEXACT);
-    if (ret)
-        return ret;
+    if (!c->waiting_pics)
+        ret = flac_finish_header(s);
 
-    /* The command line flac encoder defaults to placing a seekpoint
-     * every 10s.  So one might add padding to allow that later
-     * but there seems to be no simple way to get the duration here.
-     * So just add the amount requested by the user. */
-    if (padding)
-        flac_write_block_padding(s->pb, padding, 1);
+    return ret;
+}
+
+static int flac_write_audio_packet(struct AVFormatContext *s, AVPacket *pkt)
+{
+    FlacMuxerContext *c = s->priv_data;
+    uint8_t *streaminfo;
+    int streaminfo_size;
 
+    /* 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);
+
+        c->streaminfo = av_malloc(FLAC_STREAMINFO_SIZE);
+        if (!c->streaminfo)
+            return AVERROR(ENOMEM);
+        memcpy(c->streaminfo, streaminfo, FLAC_STREAMINFO_SIZE);
+    }
+
+    if (pkt->size)
+        avio_write(s->pb, pkt->data, pkt->size);
+    return 0;
+}
+
+static int flac_queue_flush(AVFormatContext *s)
+{
+    FlacMuxerContext *c = s->priv_data;
+    AVPacket pkt;
+    int ret = 0, write = 1;
+
+    flac_finish_header(s);
+
+    while (c->queue) {
+        ff_packet_list_get(&c->queue, &c->queue_end, &pkt);
+        if (write && (ret = flac_write_audio_packet(s, &pkt)) < 0)
+            write = 0;
+        av_packet_unref(&pkt);
+    }
     return ret;
 }
 
@@ -142,7 +310,18 @@  static int flac_write_trailer(struct AVFormatContext *s)
     int64_t file_size;
     FlacMuxerContext *c = s->priv_data;
     uint8_t *streaminfo = c->streaminfo ? c->streaminfo :
-                                          s->streams[0]->codecpar->extradata;
+                                          s->streams[c->audio_stream_idx]->codecpar->extradata;
+    int i;
+
+    if (c->waiting_pics) {
+        av_log(s, AV_LOG_WARNING, "No packets were sent for some of the "
+               "attached pictures.\n");
+        flac_queue_flush(s);
+    }
+
+    for (i = 0; i < c->nb_pics; i++)
+        av_packet_unref(&c->pics[i]);
+    av_freep(&c->pics);
 
     if (!c->write_header || !streaminfo)
         return 0;
@@ -166,23 +345,46 @@  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;
+    int ret;
 
-    /* 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);
+    if (pkt->stream_index == c->audio_stream_idx) {
+        if (c->waiting_pics) {
+            /* buffer audio packets until we get all the pictures */
+            ret = ff_packet_list_put(&c->queue, &c->queue_end, pkt, FF_PACKETLIST_FLAG_REF_PACKET);
+            if (ret < 0) {
+                av_log(s, AV_LOG_ERROR, "Out of memory in packet queue; skipping attached pictures\n");
+                c->waiting_pics = 0;
+                ret = flac_queue_flush(s);
+                if (ret < 0)
+                    return ret;
+                return flac_write_audio_packet(s, pkt);
+            }
+        } else
+            return flac_write_audio_packet(s, pkt);
+    } else {
+        int index = pkt->stream_index;
 
-        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;
 }
 
@@ -205,7 +407,7 @@  AVOutputFormat ff_flac_muxer = {
     .mime_type         = "audio/x-flac",
     .extensions        = "flac",
     .audio_codec       = AV_CODEC_ID_FLAC,
-    .video_codec       = AV_CODEC_ID_NONE,
+    .video_codec       = AV_CODEC_ID_PNG,
     .write_header      = flac_write_header,
     .write_packet      = flac_write_packet,
     .write_trailer     = flac_write_trailer,