[FFmpeg-devel] [PATCH] libavformat: palettized QuickTime video in Matroska, round 4

[Clément Bœsch]

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>