[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