[FFmpeg-devel] [PATCH] libavformat: palettized QuickTime video in Matroska, round 4
Clément Bœsch
u at pkh.me
Wed Dec 23 11:44:12 CET 2015
On Wed, Dec 23, 2015 at 11:25:44AM +0100, Mats Peterson wrote:
[...]
> From: Mats Peterson <matsp888 at yahoo.com>
So are you the sole author of this?
> Date: Wed, 23 Dec 2015 11:19:06 +0100
> Subject: [PATCH] libavformat: palettized QuickTime video in Matroska, round 4
>
Use "[PATCH v4]" instead of the suffix as you did, otherwise it will end
up in the final commit message. Also, correct prefix would be "lavf: ..."
> ---
> libavformat/Makefile | 1 +
> libavformat/matroskadec.c | 30 ++++++++++-
> libavformat/mov.c | 92 ++++++++-------------------------
> libavformat/qtpalette.c | 124 +++++++++++++++++++++++++++++++++++++++++++++
> libavformat/qtpalette.h | 4 ++
> 5 files changed, 178 insertions(+), 73 deletions(-)
> create mode 100644 libavformat/qtpalette.c
>
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index 110e9e3..e03c73e 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -18,6 +18,7 @@ OBJS = allformats.o \
> mux.o \
> options.o \
> os_support.o \
> + qtpalette.o \
> riff.o \
> sdp.o \
> url.o \
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index c574749..28bc44f 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -64,6 +64,8 @@
> #include <zlib.h>
> #endif
>
> +#include "qtpalette.h"
> +
> typedef enum {
> EBML_NONE,
> EBML_UINT,
> @@ -312,6 +314,9 @@ typedef struct MatroskaDemuxContext {
>
> /* WebM DASH Manifest live flag/ */
> int is_live;
> +
> + uint32_t palette[256];
nit: AVPALETTE_COUNT
[...]
> + if (ff_get_qtpalette(codec_id, track->codec_priv.data + 16,
> + NULL, matroska->palette)) {
nit: vertical align, more below
> + bit_depth &= 0x1F;
> + /* Behave like V_MS/VFW/FOURCC; copy the palette to
> + * extradata */
> + if (! (extradata = av_malloc(AVPALETTE_SIZE)))
> + return AVERROR(ENOMEM);
Use ff_alloc_extradata() or pad your extradata with
AV_INPUT_BUFFER_PADDING_SIZE
also, no space after '!', more below
[...]
> diff --git a/libavformat/qtpalette.c b/libavformat/qtpalette.c
> new file mode 100644
> index 0000000..90268e1
> --- /dev/null
> +++ b/libavformat/qtpalette.c
> @@ -0,0 +1,124 @@
> +/*
> + * QuickTime palette handling
> + * Copyright (c) 2001 Fabrice Bellard
> + * Copyright (c) 2009 Baptiste Coudurier <baptiste dot coudurier at gmail dot com>
> + * Copyright (c) 2015 Mats Peterson
> + *
> + * 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
> + */
> +
> +#include <stdio.h>
> +#include <inttypes.h>
Don't you just need stdint.h?
> +
> +#include "libavcodec/avcodec.h"
> +#include "libavformat/avio.h"
This file is already in libavformat.
> +#include "libavutil/intreadwrite.h"
> +#include "qtpalette.h"
> +
> +int ff_get_qtpalette(int codec_id, uint8_t *stsd, AVIOContext *pb,
> + uint32_t *palette)
stsd data isn't touched, so you can mark it const
[...]
> + if ((color_start <= 255) && (color_end <= 255)) {
> + uint8_t *p = stsd + 78;
> + for (i = color_start; i <= color_end; i++) {
> + /* each A, R, G, or B component is 16 bits;
> + * only use the top 8 bits */
> + if (pb) {
> + a = avio_r8(pb);
> + avio_r8(pb);
> + r = avio_r8(pb);
> + avio_r8(pb);
> + g = avio_r8(pb);
> + avio_r8(pb);
> + b = avio_r8(pb);
> + avio_r8(pb);
> + } else {
> + a = *p++; p++;
> + r = *p++; p++;
> + g = *p++; p++;
> + b = *p++; p++;
I see no read boundary checks, and it frighten me.
> + }
> + palette[i] = (a << 24 ) | (r << 16) | (g << 8) | (b);
a << 24 is undefined if a msb is set. Try git grep '#define RGBA('.
> + }
> + }
> + }
> +
> + return 1;
> + }
> +
> + return 0;
Usually, you have 0 for success, and < 0 (AVERROR*) for error.
> +}
> diff --git a/libavformat/qtpalette.h b/libavformat/qtpalette.h
> index 7d6802f..8f875ae 100644
> --- a/libavformat/qtpalette.h
> +++ b/libavformat/qtpalette.h
> @@ -24,6 +24,7 @@
> #define AVFORMAT_QTPALETTE_H
>
> #include <inttypes.h>
> +#include "libavformat/avio.h"
This file is already in libavformat.
[...]
--
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151223/e695f2a9/attachment.sig>
More information about the ffmpeg-devel
mailing list