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

[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 <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?

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171125/1cbf9d54/attachment.sig>