[FFmpeg-devel] [PATCH] X-Face image encoder and decoder
Stefano Sabatini
stefasab at gmail.com
Sun Oct 14 01:30:33 CEST 2012
On date Saturday 2012-10-13 22:54:02 +0200, Clément Bœsch encoded:
> 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?
Fixed.
> [...]
> > 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?
Well, the point is that there is any documentation apart the
libcompface source.
> > + */
> > +
> > +#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)
Works for me.
> > + if (i == b->nb_words && c) {
> > + b->nb_words++;
> > + *w = (uint16_t)(c & XFACE_WORDMASK);
>
> `c' looks already uint16_t, is that cast needed?
Removed.
>
> > + }
> > +}
> > +
> > +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
Wow, I missed it after so many reviews.
>
> > + 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
It was sneaky.
> > + /* 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
Removed.
> [...]
> > +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 ';'
changed
>
> > + /*
> > + 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 ':'
Fixed.
>
> > + 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;
> }
I'm keeping the original code for debuggability and for consistency
with the original source, I could rethink it after the first push.
>
> > + }
> > + }
> > +}
> > 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"
Fixed.
> [...]
> > 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;
> ...
changed
>
> [...]
> > +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.
Fixed.
> > + 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;
>
> ?
changed to:
xface->bitmap[i++] = !!(buf[j] & (0x80>>k));
which I consider a bit more intelligible (and consistent with what I
did in xface.c).
>
> 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.
Patch updated.
--
FFmpeg = Freak and Forgiving Martial Powerful Extreme Guru
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavc-add-xface-image-decoder-and-encoder.patch
Type: text/x-diff
Size: 36247 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121014/e9ced5b8/attachment.bin>
More information about the ffmpeg-devel
mailing list