On Sun, Sep 15, 2013 at 07:24:24PM -0400, Justin Ruggles wrote:
> ---
> I just squashed the two patches together for easier review. Aneesh is
> included in the Copyright header and as one of the @author for his
> contributions.
>
> Changelog | 1 +
> configure | 1 +
> doc/general.texi | 2 +
> libavcodec/Makefile | 1 +
> libavcodec/allcodecs.c | 1 +
> libavcodec/avcodec.h | 1 +
> libavcodec/codec_desc.c | 8 +
> libavcodec/mathops.h | 9 +
> libavcodec/version.h | 2 +-
> libavcodec/vp8.c | 18 +-
> libavcodec/vp8.h | 7 +
> libavcodec/webp.c | 1327
> +++++++++++++++++++++++++++++++++++++++++++++++
> libavformat/img2.c | 1 +
> 13 files changed, 1369 insertions(+), 10 deletions(-)
> create mode 100644 libavcodec/webp.c
>
[...]
> diff --git a/libavcodec/mathops.h b/libavcodec/mathops.h
> index 8a2ce90..349dd00 100644
> --- a/libavcodec/mathops.h
> +++ b/libavcodec/mathops.h
> @@ -224,4 +224,13 @@ static inline av_const unsigned int ff_sqrt(unsigned int
> a)
> return b - (a < b * b);
> }
>
> +static inline int8_t ff_u8_to_s8(uint8_t a)
> +{
> + union {
> + uint8_t u8;
> + int8_t s8;
> + } b = { .u8 = a };
> + return b.s8;
> +}
> +
> #endif /* AVCODEC_MATHOPS_H */
That might deserve a separate patch but whatever.
[...]
> diff --git a/libavcodec/webp.c b/libavcodec/webp.c
> new file mode 100644
> index 0000000..0467829
> --- /dev/null
> +++ b/libavcodec/webp.c
> @@ -0,0 +1,1327 @@
> +/*
> + * WebP (.webp) image decoder
> + * Copyright (c) 2013 Aneesh Dogra <[email protected]>
> + * Copyright (c) 2013 Justin Ruggles <[email protected]>
> + *
> + * This file is part of Libav.
> + *
> + * Libav 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.
> + *
> + * Libav 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 Libav; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
> USA
> + */
> +
> +/**
> + * @file
> + * WebP image decoder
> + *
> + * @author Aneesh Dogra <[email protected]>
> + * Container and Lossy decoding
> + *
> + * @author Justin Ruggles <[email protected]>
> + * Lossless decoder
> + *
> + * Implemented
> + * - Basic lossy decoding via the VP8 decoder
> + * - Full lossless (VP8L) decoder
> + *
> + * Unimplemented
> + * - Compressed alpha for lossy
> + * - Alpha filtering for lossy
> + * - Animation
> + * - ICC profile
> + * - Exif and XMP metadata
> + */
> +
[...]
> +static av_always_inline void color_cache_put(ImageContext *img, uint32_t c)
> +{
> + uint32_t cache_idx = (0x1E35A7BD * c) >> (32 - img->color_cache_bits);
> + AV_WB32(&img->color_cache[4 * cache_idx], c);
> +}
If I'm not mistaken, color_cache is operated as a whole triplet anywhere, so
you can simply declare it as uint32_t*.
> +/* PRED_MODE_BLACK */
> +static void inv_predict_0(uint8_t *p, const uint8_t *p_l, const uint8_t
> *p_tl,
> + const uint8_t *p_t, const uint8_t *p_tr)
> +{
> + static const uint8_t black[4] = { 0xFF, 0x00, 0x00, 0x00 };
> +
> + AV_COPY32(p, black);
nit: I'd rather use AV_WB32(p, 0xFF000000) or whatever
[...]
> +/* PRED_MODE_SELECT */
> +static void inv_predict_11(uint8_t *p, const uint8_t *p_l, const uint8_t
> *p_tl,
> + const uint8_t *p_t, const uint8_t *p_tr)
> +{
> + int diff = (abs(p_l[0] - p_tl[0]) - abs(p_t[0] - p_tl[0])) +
> + (abs(p_l[1] - p_tl[1]) - abs(p_t[1] - p_tl[1])) +
> + (abs(p_l[2] - p_tl[2]) - abs(p_t[2] - p_tl[2])) +
> + (abs(p_l[3] - p_tl[3]) - abs(p_t[3] - p_tl[3]));
nit: FFABS() maybe?
> + if (diff <= 0)
> + AV_COPY32(p, p_t);
> + else
> + AV_COPY32(p, p_l);
> +}
> +
> +/* PRED_MODE_ADD_SUBTRACT_FULL */
> +static void inv_predict_12(uint8_t *p, const uint8_t *p_l, const uint8_t
> *p_tl,
> + const uint8_t *p_t, const uint8_t *p_tr)
> +{
> + p[0] = av_clip_uint8(p_l[0] + p_t[0] - p_tl[0]);
> + p[1] = av_clip_uint8(p_l[1] + p_t[1] - p_tl[1]);
> + p[2] = av_clip_uint8(p_l[2] + p_t[2] - p_tl[2]);
> + p[3] = av_clip_uint8(p_l[3] + p_t[3] - p_tl[3]);
> +}
> +
> +static av_always_inline uint8_t clamp_add_subtract_half(int a, int b, int c)
> +{
> + int d = a + b >> 1;
> + return av_clip_uint8(d + (d - c) / 2);
> +}
nit: d + ((d - c) >> 1) for consistency?
> +static av_always_inline uint8_t color_transform_delta(uint8_t color_pred,
> + uint8_t color)
> +{
> + return ff_u8_to_s8(color_pred) * ff_u8_to_s8(color) >> 5;
> +}
I'm not sure about range expansion here
(i.e. (int)ff_u8_to_s8() * ff_u8_to_s8() >> 5)
[...]
in general looks quite good to me
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
