[FFmpeg-devel] [PATCH] X-Face image encoder and decoder
Clément Bœsch
ubitux at gmail.com
Sat Oct 13 22:54:02 CEST 2012
On Sat, Oct 13, 2012 at 08:40:14PM +0200, Stefano Sabatini wrote:
> On date Saturday 2012-10-13 19:50:27 +0200, Stefano Sabatini encoded:
> > On date Saturday 2012-10-13 01:44:43 +0200, Stefano Sabatini encoded:
> > [...]
> > > Patch updated.
> > >
> > > Note: what's the best way to check encoder+decoder in FATE? Also
> > > what's the recommended way to add an xface test? Would this require to
> > > add a sample to SAMPLES, or should we rely on the encoder?.
> >
> > Updated patch.
> >
> > I'm also attaching the patch for adding a FATE test, together with
> > sample to upload.
>
> Updated with a few fixes spotted by durandal on IRC.
>
> I'll push both patches in a few days if I read no more comments.
> --
> FFmpeg = Fierce Faithful Maxi Pacific Evil Guide
> From 4cf2decef5f75ff9b3058f51973ec4a43bfb7284 Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefasab at gmail.com>
> Date: Wed, 25 Jul 2012 12:23:31 +0200
> Subject: [PATCH] lavc: add xface image decoder and encoder
>
> Based on libcompface code by James Ashton <James.Ashton at anu.edu.au>, and
> relicensed as LGPL.
> ---
> libavcodec/Makefile | 2 +
> libavcodec/allcodecs.c | 1 +
> libavcodec/avcodec.h | 1 +
> libavcodec/codec_desc.c | 6 +
> libavcodec/xface.c | 385 +++++++++++++++++++++++++++++++++++++++++++++++
> libavcodec/xface.h | 95 ++++++++++++
> libavcodec/xfacedec.c | 211 ++++++++++++++++++++++++++
> libavcodec/xfaceenc.c | 237 +++++++++++++++++++++++++++++
> libavformat/img2.c | 1 +
> libavformat/img2enc.c | 2 +-
> 10 files changed, 940 insertions(+), 1 deletions(-)
> create mode 100644 libavcodec/xface.c
> create mode 100644 libavcodec/xface.h
> create mode 100644 libavcodec/xfacedec.c
> create mode 100644 libavcodec/xfaceenc.c
>
No general.texi updates?
[...]
> diff --git a/libavcodec/xface.c b/libavcodec/xface.c
> new file mode 100644
> index 0000000..08f09fd
> --- /dev/null
> +++ b/libavcodec/xface.c
> @@ -0,0 +1,385 @@
> +/*
> + * Copyright (c) 1990 James Ashton - Sydney University
> + * Copyright (c) 2012 Stefano Sabatini
> + *
> + * 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
> + * X-Face common data and utilities definition.
Would it make sense here (or in another @file) an URL to a page about this
this format/codec?
> + */
> +
> +#include <stdint.h>
> +#include "xface.h"
> +
> +void ff_big_add(BigInt *b, uint8_t a)
> +{
> + int i;
> + uint8_t *w;
> + uint16_t c;
> +
> + a &= XFACE_WORDMASK;
> + if (a == 0)
> + return;
> + i = 0;
> + w = b->words;
> + c = a;
> + while (i < b->nb_words && c) {
> + c += (uint16_t)*w;
> + *w++ = (uint8_t)(c & XFACE_WORDMASK);
is that cast needed?
> + c >>= XFACE_BITSPERWORD;
> + i++;
> + }
that loop could be a for loop and save a few lines (moving only the `i'
related stuff in it, no all the `c' things)
> + if (i == b->nb_words && c) {
> + b->nb_words++;
> + *w = (uint16_t)(c & XFACE_WORDMASK);
`c' looks already uint16_t, is that cast needed?
> + }
> +}
> +
> +void ff_big_div(BigInt *b, uint8_t a, uint8_t *r)
> +{
> + int i;
> + uint8_t *w;
> + uint16_t c, d;
> +
> + a &= XFACE_WORDMASK;
> + if (a == 1 || b->nb_words == 0) {
> + *r = 0;
> + return;
> + }
> +
> + /* treat this as a == WORDCARRY and just shift everything right a WORD */
> + if (a == 0) {
http://cdn.memegenerator.net/instances/400x/28304611.jpg
> + i = --b->nb_words;
> + w = b->words;
> + *r = *w;
> + while (i--) {
> + *w = *(w + 1);
> + w++;
> + }
> + *w = 0;
> + return;
> + }
> + w = b->words + (i = b->nb_words);
> + c = 0;
> + while (i--) {
> + c <<= XFACE_BITSPERWORD;
> + c += (uint16_t)*--w;
> + d = c / (uint16_t)a;
> + c = c % (uint16_t)a;
> + *w = (uint8_t)(d & XFACE_WORDMASK);
> + }
> + *r = c;
> + if (b->words[b->nb_words - 1] == 0)
> + b->nb_words--;
> +}
> +
> +void ff_big_mul(BigInt *b, uint8_t a)
> +{
> + int i;
> + uint8_t *w;
> + uint16_t c;
> +
> + a &= XFACE_WORDMASK;
> + if (a == 1 || b->nb_words == 0)
> + return;
> + if (a == 0) {
Gandalf doesn't like this one as well
> + /* treat this as a == WORDCARRY and just shift everything left a WORD */
> + i = b->nb_words++;
> + w = b->words + i;
> + while (i--) {
> + *w = *(w - 1);
> + w--;
> + }
> + *w = 0;
> + return;
> + }
> + i = b->nb_words;
> + w = b->words;
> + c = 0;
> + while (i--) {
> + c += (uint16_t)*w * (uint16_t)a;
> + *(w++) = (uint8_t)(c & XFACE_WORDMASK);
> + c >>= XFACE_BITSPERWORD;
> + }
> + if (c) {
> + b->nb_words++;
> + *w = (uint16_t)(c & XFACE_WORDMASK);
Looks like another pointless cast
[...]
> +void ff_xface_generate_face(uint8_t *dst, uint8_t * const src)
> +{
> + int m, l, k, j, i, h;
> +
> + for (j = 0; j < XFACE_HEIGHT; j++) {
> + for (i = 0; i < XFACE_WIDTH; i++) {
> + h = i + j * XFACE_WIDTH;
> + k = 0;
> +
> + /*
> + Compute k, encoding the bits *before* the current one, contained in the
> + image buffer. That is, given the grid:
> +
> + l i
> + | |
> + v v
> + +--+--+--+--+--+
> + m -> | 1| 2| 3| 4| 5|
> + +--+--+--+--+--+
> + | 6| 7| 8| 9|10|
> + +--+--+--+--+--+
> + j -> |11|12| *| | |
> + +--+--+--+--+--+
> +
> + the value k for the pixel marked as "*" will contain the bit encoding of
> + the values in the matrix marked from "1" to "12". In case the pixel is
> + near the border of the grid, the number of values contained within the
> + grid will be lesser than 12.
> + */
> +
> + for (l = i - 2; l <= i + 2; l++) {
> + for (m = j - 2; m <= j; m++) {
> + if (l >= i && m == j)
> + continue;
> + if (l > 0 && l <= XFACE_WIDTH && m > 0)
> + k = 2*k + src[l + m * XFACE_WIDTH];
> + }
> + }
> +
> +#define GEN(table) dst[h] ^= !!(table[k>>3] & 0x80>>(k&7)); break;
> +
nit: I'd be more comfortable with the break out of this macro. and if you
insist on keeping it, please remove the ';'
> + /*
> + Use the guess for the given position and the computed value of k.
> +
> + The following table shows the number of digits in k, depending on
> + the position of the pixel, and shows the corresponding guess table
> + to use:
> +
> + i=1 i=2 i=3
> + +----+----+----+
> + j=1 | 0 | 1 | 2 |
> + |g22 |g12 |g02 |
> + +----+----+----+
> + j=2 | 3 | 5 | 7 |
> + |g21 |g11 |g01 |
> + +----+----+----+
> + j=3 | 5 | 9 | 12 |
> + |g20 |g10 |g00 |
> + +----+----+----+
> + */
> +
> + switch (i) {
> + case 1:
> + switch (j) {
> + case 1: GEN(g_22);
> + case 2: GEN(g_21);
> + default: GEN(g_20);
> + }
> + break;
> + case 2:
> + switch (j) {
> + case 1: GEN(g_12);
> + case 2: GEN(g_11);
> + default: GEN(g_10);
> + }
> + break;
> + case XFACE_WIDTH - 1 :
> + switch (j) {
> + case 1: GEN(g_42);
> + case 2: GEN(g_41);
> + default: GEN(g_40);
> + }
> + break;
> + case XFACE_WIDTH :
> + switch (j) {
> + case 1: GEN(g_32);
> + case 2: GEN(g_31);
> + default: GEN(g_30);
> + }
> + break;
> + default :
nit++: here and a few times above you have a space before ':'
> + switch (j) {
> + case 1: GEN(g_02);
> + case 2: GEN(g_01);
> + default: GEN(g_00);
> + }
> + break;
> + }
untested proposition:
#define WHATEVERNAME(n) do { \
switch (j) { \
case 1: GEN(g_#n#2); break; \
case 2: GEN(g_#n#1); break; \
default: GEN(g_#n#0); break; \
} \
} while (0)
switch (i) {
case 1: WHATEVERNAME(2); break;
case 2: WHATEVERNAME(1); break;
case XFACE_WIDTH-1: WHATEVERNAME(4); break;
case XFACE_WIDTH: WHATEVERNAME(3); break;
default: WHATEVERNAME(0); break;
}
> + }
> + }
> +}
> diff --git a/libavcodec/xface.h b/libavcodec/xface.h
> new file mode 100644
> index 0000000..d7ffd3f
> --- /dev/null
> +++ b/libavcodec/xface.h
> @@ -0,0 +1,95 @@
> +/*
> + * Copyright (c) 1990 James Ashton - Sydney University
> + * Copyright (c) 2012 Stefano Sabatini
> + *
> + * 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
> + * X-Face common definitions.
> + */
> +
> +#include <stdint.h>
> +
> +/* define the face size - 48x48x1 */
> +#define XFACE_WIDTH 48
> +#define XFACE_HEIGHT 48
> +#define XFACE_PIXELS (XFACE_WIDTH * XFACE_HEIGHT)
> +
> +/* compressed output uses the full range of printable characters.
> + * in ASCII these are in a contiguous block so we just need to know
nit: "In"
[...]
> diff --git a/libavcodec/xfacedec.c b/libavcodec/xfacedec.c
> new file mode 100644
> index 0000000..46ff560
> --- /dev/null
> +++ b/libavcodec/xfacedec.c
> @@ -0,0 +1,211 @@
> +/*
> + * Copyright (c) 1990 James Ashton - Sydney University
> + * Copyright (c) 2012 Stefano Sabatini
> + *
> + * 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
> + * X-Face decoder, based on libcompface, by James Ashton.
> + */
> +
> +#include "libavutil/pixdesc.h"
> +#include "avcodec.h"
> +#include "bytestream.h"
> +#include "xface.h"
> +
> +static int pop_integer(BigInt *b, ProbRange *pranges)
> +{
> + uint8_t r;
> + int i;
> +
> + /* extract the last byte into v, and shift right b by 8 bits */
> + ff_big_div(b, 0, &r);
> +
> + i = 0;
> + while (r < pranges->offset || r >= pranges->range + pranges->offset) {
> + pranges++;
> + i++;
> + }
> + ff_big_mul(b, pranges->range);
> + ff_big_add(b, r - pranges->offset);
> + return i;
> +}
> +
> +static void pop_greys(BigInt *b, char *bitmap, int w, int h)
> +{
> + if (w > 3) {
> + w /= 2;
> + h /= 2;
> + pop_greys(b, bitmap, w, h);
> + pop_greys(b, bitmap + w, w, h);
> + pop_greys(b, bitmap + XFACE_WIDTH * h, w, h);
> + pop_greys(b, bitmap + XFACE_WIDTH * h + w, w, h);
> + } else {
> + w = pop_integer(b, ff_xface_probranges_2x2);
> + if (w & 1)
> + *bitmap = 1;
> + if (w & 2)
> + *(bitmap + 1) = 1;
> + if (w & 4)
> + *(bitmap + XFACE_WIDTH) = 1;
> + if (w & 8)
> + *(bitmap + XFACE_WIDTH + 1) = 1;
nit:
bitmap[0] = 1;
bitmap[1] = 1;
bitmap[XFACE_WIDTH] = 1;
...
[...]
> +static int xface_decode_frame(AVCodecContext *avctx,
> + void *data, int *data_size,
> + AVPacket *avpkt)
> +{
> + XFaceContext *xface = avctx->priv_data;
> + int ret, i, j, k;
> + uint8_t byte;
> + BigInt b = {0};
> + char *buf;
> + int64_t c;
> +
> + if (xface->frame.data[0])
> + avctx->release_buffer(avctx, &xface->frame);
> + xface->frame.data[0] = NULL;
> + if ((ret = avctx->get_buffer(avctx, &xface->frame)) < 0)
> + return ret;
> + xface->frame.reference = 0;
> +
> + for (i = 0, k = 0; avpkt->data[i] && i < avpkt->size; i++) {
> + c = avpkt->data[i];
> +
> + /* ignore invalid digits */
> + if (c < XFACE_FIRST_PRINT || c > XFACE_LAST_PRINT)
> + continue;
> +
> + if (++k > XFACE_MAX_DIGITS) {
> + av_log(avctx, AV_LOG_WARNING,
> + "Buffer is longer than expected, truncating at byte %i\n", i);
> + break;
> + }
> + ff_big_mul(&b, XFACE_PRINTS);
> + ff_big_add(&b, c - XFACE_FIRST_PRINT);
> + }
> +
> + /* decode image and put it in bitmap */
> + memset(xface->bitmap, 0, XFACE_PIXELS);
> + buf = xface->bitmap;
> + decode_block(&b, buf, 16, 16, 0);
> + decode_block(&b, buf + 16, 16, 16, 0);
> + decode_block(&b, buf + 32, 16, 16, 0);
> + decode_block(&b, buf + XFACE_WIDTH * 16, 16, 16, 0);
> + decode_block(&b, buf + XFACE_WIDTH * 16 + 16, 16, 16, 0);
> + decode_block(&b, buf + XFACE_WIDTH * 16 + 32, 16, 16, 0);
> + decode_block(&b, buf + XFACE_WIDTH * 32 , 16, 16, 0);
> + decode_block(&b, buf + XFACE_WIDTH * 32 + 16, 16, 16, 0);
> + decode_block(&b, buf + XFACE_WIDTH * 32 + 32, 16, 16, 0);
> +
> + ff_xface_generate_face(xface->bitmap, xface->bitmap);
> +
> + /* convert image from 1=black 0=white bitmap to MONOW */
MONOW(hite?)
[...]
> +static int xface_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
> + const AVFrame *frame, int *got_packet)
> +{
> + XFaceContext *xface = avctx->priv_data;
> + ProbRangesQueue pq = {{ 0 }, 0};
> + uint8_t bitmap_copy[XFACE_PIXELS];
> + BigInt b = {0};
> + int i, j, k, ret = 0;
> + uint8_t *buf, *p;
> + char intbuf[XFACE_MAX_DIGITS];
> +
> + if (avctx->width || avctx->height) {
> + if (avctx->width != XFACE_WIDTH || avctx->height != XFACE_HEIGHT) {
> + av_log(avctx, AV_LOG_ERROR,
> + "Size value %dx%d not supported, only accepts a size of %dx%d\n",
> + avctx->width, avctx->height, XFACE_WIDTH, XFACE_HEIGHT);
I think I've already see this message a bit earlier, but with hardcode of
the size; you might want to keep one or another.
> + return AVERROR(EINVAL);
> + }
> + }
> + avctx->width = XFACE_WIDTH;
> + avctx->height = XFACE_HEIGHT;
> +
> + /* convert image from MONOWHITE to 1=black 0=white bitmap */
> + buf = frame->data[0];
> + for (i = 0, j = 0; i < XFACE_PIXELS; ) {
> + for (k = 0; k < 8; k++)
> + xface->bitmap[i++] = !!((1<<(7-k)) & buf[j]);
Wouldn't it be faster/simpler to do something like (untested):
xface->bitmap[i++] = buf[j]>>(7-k) & 1;
?
And BTW, I think you can simplify the GEN() the same way (if you do so,
don't forget to update the comment(s?) where you put the formula)
[...]
No more comments from me, LGTM overall.
--
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121013/158f5ce5/attachment.asc>
More information about the ffmpeg-devel
mailing list