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

[Rostislav Pehlivanov]

On 2 August 2017 at 15:30, James Almer  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(>pkt, pkt);
> > +if (ret < 0) {
> > +av_freep();
> > +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,
> > - _size);
> > -if (streaminfo && streaminfo_size == FLAC_STREAMINFO_SIZE) {
> > -av_freep(>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(>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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

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(>pkt, pkt);
> +if (ret < 0) {
> +av_freep();
> +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,
> - _size);
> -if (streaminfo && streaminfo_size == FLAC_STREAMINFO_SIZE) {
> -av_freep(>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(>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.

> +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 +420,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,
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rodger Combs]

---
 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
--- 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,156 @@ 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);
 }
-if (par->codec_id != AV_CODEC_ID_FLAC) {
-av_log(s, AV_LOG_ERROR, "unsupported codec\n");
+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 (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, >pics[i]);
+if (ret)
+return ret;
+}
+
+ret = flac_write_block_comment(s->pb, >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 

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

[Rodger Combs]

---
 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
--- 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,156 @@ 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);
 }
-if (par->codec_id != AV_CODEC_ID_FLAC) {
-av_log(s, AV_LOG_ERROR, "unsupported codec\n");
+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 (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, >pics[i]);
+if (ret)
+return ret;
+}
+
+ret = flac_write_block_comment(s->pb, >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 

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

[James Almer]

On 5/8/2017 1:36 AM, Rodger Combs wrote:
> ---
>  libavformat/flacenc.c | 271 
> +++---
>  1 file changed, 236 insertions(+), 35 deletions(-)
> 
> diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
> index b894f9e..9bb4947 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,149 @@ 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);
>  }
> -if (par->codec_id != AV_CODEC_ID_FLAC) {
> -av_log(s, AV_LOG_ERROR, "unsupported codec\n");
> +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)) {
> +av_log(s, AV_LOG_ERROR, "Duplicate attachment for type '%s'\n", 
> ff_id3v2_picture_types[type]);
>  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, >pics[i]);
> +if (ret)
> +return ret;
> +}
> +
> +ret = flac_write_block_comment(s->pb, >metadata, !padding,
> +   s->flags & AVFMT_FLAG_BITEXACT);
> +if (ret)
> +return ret;
> +
> +/* The command line flac encoder defaults to 

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

[Rostislav Pehlivanov]

On 8 May 2017 at 05:36, Rodger Combs  wrote:

> ---
>  libavformat/flacenc.c | 271 ++
> +---
>  1 file changed, 236 insertions(+), 35 deletions(-)
>
> diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
> index b894f9e..9bb4947 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,149 @@ 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);
>  }
> -if (par->codec_id != AV_CODEC_ID_FLAC) {
> -av_log(s, AV_LOG_ERROR, "unsupported codec\n");
> +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)) {
> +av_log(s, AV_LOG_ERROR, "Duplicate attachment for type '%s'\n",
> ff_id3v2_picture_types[type]);
>  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, >pics[i]);
> +if (ret)
> +return ret;
> +}
> +
> +ret = flac_write_block_comment(s->pb, >metadata, !padding,
> +   s->flags & AVFMT_FLAG_BITEXACT);
> +if (ret)
> +return ret;
> +
> +/* The command line flac encoder 

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

[Michael Niedermayer]

On Sun, May 07, 2017 at 11:36:22PM -0500, Rodger Combs wrote:
> ---
>  libavformat/flacenc.c | 271 
> +++---
>  1 file changed, 236 insertions(+), 35 deletions(-)

Didnt review but i can confirm that it seems to work

thx

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates


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

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

[Rodger Combs]

---
 libavformat/flacenc.c | 271 +++---
 1 file changed, 236 insertions(+), 35 deletions(-)

diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
index b894f9e..9bb4947 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,149 @@ 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);
 }
-if (par->codec_id != AV_CODEC_ID_FLAC) {
-av_log(s, AV_LOG_ERROR, "unsupported codec\n");
+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)) {
+av_log(s, AV_LOG_ERROR, "Duplicate attachment for type '%s'\n", 
ff_id3v2_picture_types[type]);
 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, >pics[i]);
+if (ret)
+return ret;
+}
+
+ret = flac_write_block_comment(s->pb, >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