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

[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 <rodger.combs at gmail.com>
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> 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,
>> -                                         &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 */
>> +            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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>