[FFmpeg-devel] [PATCH] Add ICO muxer

Michael Bradshaw mbradshaw at sorensonmedia.com
Sun Aug 12 18:27:22 CEST 2012


On Sun, Aug 12, 2012 at 1:57 AM, Peter Ross <pross at xvid.org> wrote:
> On Sat, Aug 11, 2012 at 03:15:26PM -0600, Michael Bradshaw wrote:
>> On Thu, Aug 9, 2012 at 8:35 PM, Michael Bradshaw
>> <mbradshaw at sorensonmedia.com> wrote:
>> > Hi,
>> >
>> > Attached patch adds support for ICO muxing. I expect I'll need to make
>> > changes, so please don't hold back in your reviews (I've never written
>> > a muxer before so I'm still unsure if I got all the API contracts
>> > right). I'm mostly unsure of the flushing (if I didn't flush, my very
>> > small files would have missing data).
>>
>> Sorry, previous patch had an error. Attached is a revised patch (note
>> I'm still unsure of the flushing...).
>
>> From d6a930b264250e94981b66a368d68391eb6c1c77 Mon Sep 17 00:00:00 2001
>> From: Michael Bradshaw <mbradshaw at sorensonmedia.com>
>> Date: Sat, 11 Aug 2012 15:12:15 -0600
>> Subject: [PATCH] Add ICO muxer
>>
>> Signed-off-by: Michael Bradshaw <mbradshaw at sorensonmedia.com>
>> ---
>>  libavformat/Makefile     |    1 +
>>  libavformat/allformats.c |    2 +-
>>  libavformat/icoenc.c     |  223 ++++++++++++++++++++++++++++++++++++++++++++++
>
>
> Don't forget to update the documentation (specifically Changelog and doc/general.texi)

Added to Changelog and updated doc/general.texi to show ICO muxing support

>>  3 files changed, 225 insertions(+), 1 deletions(-)
>>  create mode 100644 libavformat/icoenc.c
>>
>> diff --git a/libavformat/Makefile b/libavformat/Makefile
>> index d77c9ea..d61c990 100644
>> --- a/libavformat/Makefile
>> +++ b/libavformat/Makefile
>> @@ -122,6 +122,7 @@ OBJS-$(CONFIG_H264_DEMUXER)              += h264dec.o rawdec.o
>>  OBJS-$(CONFIG_H264_MUXER)                += rawenc.o
>>  OBJS-$(CONFIG_HLS_DEMUXER)               += hls.o
>>  OBJS-$(CONFIG_ICO_DEMUXER)               += icodec.o
>> +OBJS-$(CONFIG_ICO_MUXER)                 += icoenc.o
>>  OBJS-$(CONFIG_IDCIN_DEMUXER)             += idcin.o
>>  OBJS-$(CONFIG_IDF_DEMUXER)               += bintext.o
>>  OBJS-$(CONFIG_IFF_DEMUXER)               += iff.o
>> diff --git a/libavformat/allformats.c b/libavformat/allformats.c
>> index d1b41e6..9df6280 100644
>> --- a/libavformat/allformats.c
>> +++ b/libavformat/allformats.c
>> @@ -112,7 +112,7 @@ void av_register_all(void)
>>      REGISTER_MUXDEMUX (H263, h263);
>>      REGISTER_MUXDEMUX (H264, h264);
>>      REGISTER_DEMUXER  (HLS, hls);
>> -    REGISTER_DEMUXER  (ICO, ico);
>> +    REGISTER_MUXDEMUX (ICO, ico);
>>      REGISTER_DEMUXER  (IDCIN, idcin);
>>      REGISTER_DEMUXER  (IDF, idf);
>>      REGISTER_DEMUXER  (IFF, iff);
>> diff --git a/libavformat/icoenc.c b/libavformat/icoenc.c
>> new file mode 100644
>> index 0000000..5dc93d4
>> --- /dev/null
>> +++ b/libavformat/icoenc.c
>> @@ -0,0 +1,223 @@
>> +/*
>> + * Microsoft Windows ICO muxer
>> + * Copyright (c) 2012 Michael Bradshaw <mbradshaw at sorensonmedia.com>
>> + *
>> + * This file is part of FFmpeg.
>> + *
>> + * FFmpeg is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * FFmpeg is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with FFmpeg; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>> + */
>> +
>> +/**
>> + * @file
>> + * Microsoft Windows ICO muxer
>> + */
>> +
>> +#include "libavutil/intreadwrite.h"
>> +#include "libavutil/pixdesc.h"
>> +#include "avformat.h"
>> +
>> +typedef struct {
>> +    int offset;
>> +    int size;
>> +    unsigned char width;
>> +    unsigned char height;
>> +    short bits;
>> +} IcoImage;
>> +
>> +typedef struct {
>> +    int current_image;
>> +    int nb_images;
>> +    IcoImage *images;
>> +} IcoMuxContext;
>> +
>> +static int ico_check_attributes(AVFormatContext *s, const AVCodecContext *c)
>> +{
>> +    // Supported BMP pixel formats in ICO files:
>> +    // Depth  Format
>> +    // 1bit:  pal8
>> +    // 4bit:  pal8
>> +    // 8bit:  pal8
>> +    // 16bit: rgb555le
>> +    // 24bit: bgr24
>> +    // 32bit: bgra
>> +
>> +    // Supported PNG pixel formats in ICO files:
>> +    // Depth  Format
>> +    // 32bit: rgba
>
> The information above is probably best to be put in general.texi

I looked at general.texi and I can't find a place that I think it
really fits, so I added it to muxers.texi.

>> +    if (c->codec_id == CODEC_ID_BMP) {
>> +        if (c->pix_fmt == PIX_FMT_PAL8 && PIX_FMT_RGB32 != PIX_FMT_BGRA) {
>> +            av_log(s, AV_LOG_ERROR, "Error: wrong endianness for bmp pixel format\n");
>
> Dont normally need to prefix things with 'Error: '
> This applies to this av_log(), and all others in your patch.

Fixed all of them.

>> +            return AVERROR(EINVAL);
>> +        } else if (c->pix_fmt != PIX_FMT_PAL8 && // TODO: fix error message to contain the right message
>> +                   c->pix_fmt != PIX_FMT_RGB555LE &&
>> +                   c->pix_fmt != PIX_FMT_BGR24 &&
>> +                   c->pix_fmt != PIX_FMT_BGRA) {
>> +            av_log(s, AV_LOG_ERROR, "Error: bmp must be 1bit, 4bit, 8bit, 16bit, 24bit, or 32bit\n");
>> +            return AVERROR(EINVAL);
>> +        }
>> +    } else if (c->codec_id == CODEC_ID_PNG) {
>> +        if (c->pix_fmt != PIX_FMT_RGBA) {
>> +            av_log(s, AV_LOG_ERROR, "Error: png in ico requires pixel format to be rgba\n");
>> +            return AVERROR(EINVAL);
>> +        }
>> +    } else {
>> +        av_log(s, AV_LOG_ERROR, "Error: unsupported codec %s\n", c->codec_name);
>> +        return AVERROR(EINVAL);
>> +    }
>> +
>> +    if (c->width > 256 ||
>> +        c->height > 256) {
>> +        av_log(s, AV_LOG_ERROR, "Error: unsupported dimensions %dx%d (dimensions cannot exceed 256x256)\n", c->width, c->height);
>> +        return AVERROR(EINVAL);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int ico_write_header(AVFormatContext *s)
>> +{
>> +    IcoMuxContext *ico = s->priv_data;
>> +    AVIOContext *pb = s->pb;
>> +    int ret;
>> +    int i;
>> +
>> +    if (!pb->seekable) {
>> +        av_log(s, AV_LOG_ERROR, "Error: output is not seekable\n");
>> +        return AVERROR(EINVAL);
>> +    }
>> +
>> +    ico->current_image = 0;
>> +    ico->nb_images = s->nb_streams;
>> +
>> +    avio_wl16(pb, 0); // reserved
>> +    avio_wl16(pb, 1); // 1 == icon
>> +    avio_wl16(pb, ico->nb_images);
>> +
>> +    for (i = 0; i < s->nb_streams; i++) {
>> +        if (ret = ico_check_attributes(s, s->streams[i]->codec))
>> +            return ret;
>> +
>> +        // Fill in later when writing trailer...
>> +        avio_wl64(pb, 0);
>> +        avio_wl64(pb, 0);
>
> avio_skip?

Ah, yes, that's the function I was looking for. Changed to avio_skip(pb, 16).

>> +    }
>> +
>> +    ico->images = av_mallocz(ico->nb_images * sizeof(IcoMuxContext));
>> +    if (!ico->images)
>> +        return AVERROR(ENOMEM);
>> +
>> +    avio_flush(pb);
>> +
>> +    return 0;
>> +}
>> +
>> +static int ico_write_packet(AVFormatContext *s, AVPacket *pkt)
>> +{
>> +    IcoMuxContext *ico = s->priv_data;
>> +    IcoImage *image;
>> +    AVIOContext *pb = s->pb;
>> +    AVCodecContext *c = s->streams[pkt->stream_index]->codec;
>> +    int i;
>> +
>> +    if (ico->current_image >= ico->nb_images) {
>> +        av_log(s, AV_LOG_ERROR, "Error: ico already contains %d images\n", ico->current_image);
>> +        return AVERROR(EIO);
>> +    }
>> +
>> +    image = &ico->images[ico->current_image++];
>> +
>> +    image->offset = avio_tell(pb);
>> +    image->width = (c->width == 256) ? 0 : c->width;
>> +    image->height = (c->height == 256) ? 0 : c->height;
>> +
>> +    if (c->codec_id == CODEC_ID_PNG) {
>> +        image->bits = c->bits_per_coded_sample;
>> +        image->size = pkt->size;
>> +
>> +        avio_write(pb, pkt->data, pkt->size);
>> +    } else { // BMP
>> +        if (AV_RL32(pkt->data + 14) != 40) { // must be BITMAPINFOHEADER
>> +            av_log(s, AV_LOG_ERROR, "Error: invalid bmp\n");
>> +            return AVERROR(EINVAL);
>> +        }
>> +
>> +        image->bits = AV_RL16(pkt->data + 28); // allows things like 1bit and 4bit images to be preserved
>> +        image->size = pkt->size - 14 + c->height * (c->width + 7) / 8;
>> +
>> +        avio_write(pb, pkt->data + 14, 8); // Skip the BITMAPFILEHEADER header
>> +        avio_wl32(pb, AV_RL32(pkt->data + 22) * 2); // rewrite height as 2 * height
>> +        avio_write(pb, pkt->data + 26, pkt->size - 26);
>> +
>> +        for (i = 0; i < c->height * (c->width + 7) / 8; ++i)
>> +            avio_w8(pb, 0x00); // Write bitmask (opaque)
>> +    }
>> +
>> +    avio_flush(pb);
>> +
>> +    return 0;
>> +}
>> +
>> +static int ico_write_trailer(AVFormatContext *s)
>> +{
>> +    IcoMuxContext *ico = s->priv_data;
>> +    AVIOContext *pb = s->pb;
>> +    int i;
>> +
>> +    if (ico->current_image != ico->nb_images) {
>> +        av_log(s, AV_LOG_ERROR, "Error: ico is missing %d frames\n", ico->nb_images - ico->current_image);
>> +        return AVERROR(EIO);
>
> This situation will occur when the user interrupts the muxer. It would be nice if the muxer would
> generate a working .ico file when this happens.  e.g. defer 'avio_wl16(pb, ico->nb_images)' until
> ico_write_trailer.

I was worried that it might generate a corrupt file (since in the
write_header funciton I have to make room for each image's header),
but now that I think about it I think it should be OK. I've changed it
to do that. That actually fixed a potential memory leak that I had
missed too (av_freep(&ico->images) wouldn't get called if I returned
early).

>> +    }
>> +
>> +    avio_seek(pb, 6, SEEK_SET);
>> +
>> +    for (i = 0; i < ico->nb_images; i++) {
>> +        avio_w8(pb, ico->images[i].width);
>> +        avio_w8(pb, ico->images[i].height);
>> +
>> +        if (s->streams[i]->codec->codec_id == CODEC_ID_BMP &&
>> +            s->streams[i]->codec->pix_fmt == PIX_FMT_PAL8) {
>> +            avio_w8(pb, (ico->images[i].bits >= 8) ? 0 : 1 << ico->images[i].bits);
>> +        } else {
>> +            avio_w8(pb, 0);
>> +        }
>> +
>> +        avio_w8(pb, 0); // reserved
>> +        avio_wl16(pb, 1); // color planes
>> +        avio_wl16(pb, ico->images[i].bits);
>> +        avio_wl32(pb, ico->images[i].size);
>> +        avio_wl32(pb, ico->images[i].offset);
>> +    }
>> +
>> +    avio_flush(pb);
>> +
>> +    av_freep(&ico->images);
>> +
>> +    return 0;
>> +}
>> +
>> +AVOutputFormat ff_ico_muxer = {
>> +    .name           = "ico",
>> +    .long_name      = NULL_IF_CONFIG_SMALL("Microsoft Windows ICO"),
>> +    .priv_data_size = sizeof(IcoMuxContext),
>> +    .mime_type      = "image/vnd.microsoft.icon",
>> +    .extensions     = "ico",
>> +    .audio_codec    = CODEC_ID_NONE,
>> +    .video_codec    = CODEC_ID_BMP,
>> +    .write_header   = ico_write_header,
>> +    .write_packet   = ico_write_packet,
>> +    .write_trailer  = ico_write_trailer,
>> +    .flags          = AVFMT_NOTIMESTAMPS,
>> +};
>> --
>> 1.7.7
>
> I can't really help you with the flushing problem. I thought this is only required
> if you want a partially muxed file to be usable by another process. And, in the case
> of ico, that's not possible, because the image offsets are written at the ico_write_trailer.

Yeah, without the flushing my little 1KB BMP didn't get muxed into it...

New version is attached (it also has Michael Neidermayer's requested fix).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-ICO-muxer.patch
Type: application/octet-stream
Size: 10043 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120812/ffbd70c1/attachment.obj>


More information about the ffmpeg-devel mailing list