subject:"\[FFmpeg\-devel\] \[PATCH 1\/2\] lavf\/flacenc\: support writing attached pictures"

Re: [FFmpeg-devel] [PATCH 1/2] lavf/flacenc: support writing attached pictures

2017-11-24 Thread Michael Niedermayer

On Fri, Nov 24, 2017 at 08:53:55PM -0300, James Almer wrote:
> On 11/24/2017 8:27 PM, Michael Niedermayer wrote:
> > On Thu, Nov 23, 2017 at 07:08:42PM -0300, James Almer wrote:
> >> From: Rodger Combs 
> >>
> >> Signed-off-by: James Almer 
> >> ---
> >> Should be good to commit now.
> >>
> >>  libavformat/flacenc.c | 286 
> >> +++---
> >>  1 file changed, 250 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
> >> index b894f9ef61..84da54a1df 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);
> >> +}
> > 
> > This should print the name of the codec thats not supported, so the
> > user knows what is unsupported
> > 
> > 
> > [...]
> >> @@ -166,23 +347,56 @@ 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,
> >> - _size);
> >> -if (streaminfo && streaminfo_size == FLAC_STREAMINFO_SIZE) {
> >> -av_freep(>streaminfo);
> >> +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));
> > 
> > This kind of packet buffering code should not be duplicated per
> > muxer.
> 
> Having this code in three muxers isn't much of an issue. Also, I've been
> waiting on some promised AVPacketList API but it never showed up.
> 
> > Also we already have buffering code that ensures packet interleaving.
> > can this not be done there ?
> 
> What code would this be?

see
interleave_packet()
ff_interleave_packet_per_dts()

I think waiting for the first video packet should be much simpler in
there than in a muxer, its not very different from waiting for a
packet with fitting dts


thanks

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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable


signature.asc

Re: [FFmpeg-devel] [PATCH 1/2] lavf/flacenc: support writing attached pictures

2017-11-24 Thread James Almer

On 11/24/2017 8:27 PM, Michael Niedermayer wrote:
> On Thu, Nov 23, 2017 at 07:08:42PM -0300, James Almer wrote:
>> From: Rodger Combs 
>>
>> Signed-off-by: James Almer 
>> ---
>> Should be good to commit now.
>>
>>  libavformat/flacenc.c | 286 
>> +++---
>>  1 file changed, 250 insertions(+), 36 deletions(-)
>>
>> diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
>> index b894f9ef61..84da54a1df 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);
>> +}
> 
> This should print the name of the codec thats not supported, so the
> user knows what is unsupported
> 
> 
> [...]
>> @@ -166,23 +347,56 @@ 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,
>> - _size);
>> -if (streaminfo && streaminfo_size == FLAC_STREAMINFO_SIZE) {
>> -av_freep(>streaminfo);
>> +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));
> 
> This kind of packet buffering code should not be duplicated per
> muxer.

Having this code in three muxers isn't much of an issue. Also, I've been
waiting on some promised AVPacketList API but it never showed up.

> Also we already have buffering code that ensures packet interleaving.
> can this not be done there ?

What code would this be?

> or am i missing something ?

It's just buffering audio until the first video packet (AKA, cover art)
shows up. It then writes it, dumps all the buffered audio, then keeps
muxing audio normally (no more images are expected after that, and any
that shows up will be ignored).

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

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

Re: [FFmpeg-devel] [PATCH 1/2] lavf/flacenc: support writing attached pictures

2017-11-24 Thread Michael Niedermayer

On Thu, Nov 23, 2017 at 07:08:42PM -0300, James Almer wrote:
> From: Rodger Combs 
> 
> Signed-off-by: James Almer 
> ---
> Should be good to commit now.
> 
>  libavformat/flacenc.c | 286 
> +++---
>  1 file changed, 250 insertions(+), 36 deletions(-)
> 
> diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
> index b894f9ef61..84da54a1df 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);
> +}

This should print the name of the codec thats not supported, so the
user knows what is unsupported


[...]
> @@ -166,23 +347,56 @@ 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,
> - _size);
> -if (streaminfo && streaminfo_size == FLAC_STREAMINFO_SIZE) {
> -av_freep(>streaminfo);
> +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));

This kind of packet buffering code should not be duplicated per
muxer.
Also we already have buffering code that ensures packet interleaving.
can this not be done there ?
or am i missing something ?



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

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 


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

Re: [FFmpeg-devel] [PATCH 1/2] lavf/flacenc: support writing attached pictures

2017-11-23 Thread Carl Eugen Hoyos

2017-11-23 23:08 GMT+01:00 James Almer :

> Should be good to commit now.

Please mention ticket #4442.

Thank you, Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/2] lavf/flacenc: support writing attached pictures

2017-11-23 Thread James Almer

From: Rodger Combs 

Signed-off-by: James Almer 
---
Should be good to commit now.

 libavformat/flacenc.c | 286 +++---
 1 file changed, 250 insertions(+), 36 deletions(-)

diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
index b894f9ef61..84da54a1df 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, >pics[i]);
+av_packet_unref(>pics[i]);
+if (ret)
+return ret;
+}
+
+ret = flac_write_block_comment(s->pb, >metadata, !padding,
+

Re: [FFmpeg-devel] [PATCH 1/2] lavf/flacenc: support writing attached pictures

Re: [FFmpeg-devel] [PATCH 1/2] lavf/flacenc: support writing attached pictures

Re: [FFmpeg-devel] [PATCH 1/2] lavf/flacenc: support writing attached pictures

Re: [FFmpeg-devel] [PATCH 1/2] lavf/flacenc: support writing attached pictures

[FFmpeg-devel] [PATCH 1/2] lavf/flacenc: support writing attached pictures

5 matches

Site Navigation

Mail list logo

Footer information