[Ffmpeg-devel] [PATCH] C93 demuxer and decoder (GSoC qualification task)
Michael Niedermayer
michaelni
Mon Apr 2 00:56:25 CEST 2007
Hi
On Sun, Apr 01, 2007 at 11:04:14PM +0300, Anssi Hannula wrote:
> Hi!
>
> As my Google Summer of Code qualification task, attached is a patch
> which adds C93 [1] demuxer and video decoder to FFmpeg. The samples from
> samples.mplayerhq.hu can be properly played out with it.
>
> I'm not too familiar with FFmpeg codebase yet, though, so there could be
> some silly mistakes in the code ;). Please review it.
>
> [1] http://wiki.multimedia.cx/index.php?title=C93
[...]
> +static inline int c93_conv_offset(int offset, int x, int y, int size,
> + int stride)
> +{
> + int prev_x = offset % 320 + x;
> + int prev_y = offset / 320 + y;
> + if (prev_x >= 320) {
> + prev_x -= 320;
> + prev_y += size;
> + }
> + return prev_y*stride+prev_x;
> +}
a static inline function with division and modulo executed per pixel uhm
this is not efficient especially as its redoing the same division and modulo
per pixel
also the prev_y += size; looks odd are you sure this is correct?
> +
> +static int c93_decode_frame(AVCodecContext * avctx,
> + void *data, int *data_size, uint8_t * buf, int buf_size)
> +{
> + C93DecoderContext * const c93 = avctx->priv_data;
> + AVFrame * const newpic = (AVFrame*) &(c93->pictures[c93->currentpic]);
> + AVFrame * const oldpic = (AVFrame*) &(c93->pictures[1 - c93->currentpic]);
useless casts
> + AVFrame *picture = data;
> + uint8_t *out;
> + int stride, i, x, y;
> + C93BlockType bt1, bt2;
> +
> + c93->currentpic = c93->currentpic ? 0 : 1;
c93->currentpic ^=1;
> + if (avctx->reget_buffer(avctx, newpic)) {
> + av_log(avctx, AV_LOG_ERROR, "reget_buffer() failed\n");
> + return -1;
> + }
> + if (avctx->reget_buffer(avctx, oldpic)) {
> + av_log(avctx, AV_LOG_ERROR, "reget_buffer() failed\n");
> + return -1;
> + }
i think you should set
FF_BUFFER_HINTS_VALID | FF_BUFFER_HINTS_PRESERVE | FF_BUFFER_HINTS_REUSABLE | FF_BUFFER_HINTS_READABLE
iam not sure if the double reget_buffer() is completely safe, normally
id just call it on the frame which is changed and output ...
and you NEED to run release_buffer() on codec close otherwise bad things
will happen with some applications
> +
> + stride = newpic->linesize[0];
> + out = newpic->data[0];
> +
> + newpic->reference = 1;
this must be set before *get_buffer()
> + if (buf[0] & 0x02) { /* first frame */
> + newpic->pict_type = FF_I_TYPE;
> + newpic->key_frame = 1;
> + } else {
> + newpic->pict_type = FF_P_TYPE;
> + newpic->key_frame = 0;
> + }
> +
> + if ((buf++)[0] & 0x01) { /* contains palette */
> + uint32_t *pal1 = (uint32_t *) newpic->data[1];
> + uint32_t *pal2 = (uint32_t *) oldpic->data[1];
> + for (i = 0; i < 256; i++, buf += 3) {
> + pal1[i] = pal2[i] = (buf[2] << 16) | (buf[1] << 8) | buf[0];
bytestream_get_le24()
> + }
changing the old picture is not safe it could be just drawn to the screen
by the user application in another thread
> + }
> +
> + bt1 = bt2 = 0;
> + for (x = y = 0; x < 320 || y < 184; x += 8) {
> + int offset, y_off, x_off, j, k, col0, col1;
> + int colbit = 0;
> + int cols[4];
> +
> + if (x >= 320) {
> + y += 8;
> + x = 0;
> + }
why not a y and x loop ?
> + if (!bt1) {
> + bt1 = buf[0] & 0x0F;
> + bt2 = (buf[0] >> 4) & 0x0F;
the &0x0F is useless
> + buf++;
> + }
besides
if(!bt)
bt=*buf++;
switch(bt & 0xF)
...
bt>>=4;
will work too and be simpler
> +
> + switch (bt1) {
> + case C93_8X8_FROM_PREV:
> + offset = AV_RL16(buf);
> + buf += 2;
bytestream_get_le16()
> + for (j = 0; j < 8; j++)
> + for (i = 0; i < 8; i++) {
> + int loc = c93_conv_offset(offset, i, j, 8, stride);
> + if (loc >= 192 * stride + 320) {
> + av_log(avctx, AV_LOG_ERROR, "invalid offset %d for"
> + " mode 2 at %dx%d\n", offset, x, y);
> + return -1;
> + }
stride can be negative in which case this check breaks
> + out[(y+j)*stride+x+i] = oldpic->data[0][loc];
cant memcpy() be used here instead of 1 pixel at a time copying?
[...]
> + case C93_4X4_FROM_CURR:
> + for (k = 0; k < 4; k++) {
> + y_off = k > 1 ? 4 : 0;
> + x_off = (k % 2) ? 4 : 0;
why not a y_off and x_off loop?
[...]
> + case C93_4X4_2COLOR:
> + for (k = 0; k < 4; k++) {
> + y_off = k > 1 ? 4 : 0;
> + x_off = (k % 2) ? 4 : 0;
> + col0 = buf[0];
> + col1 = buf[1];
> + buf += 2;
> + for (j = 0; j < 4; j++) {
> + for (i = 0; i < 4; i++) {
> + out[(y+j+y_off)*stride+x+i+x_off] =
> + (buf[0] & (1 << colbit++)) ? col1 : col0;
> + if (colbit > 7) {
> + buf++;
> + colbit = 0;
> + }
you can read 16bit once and avoid this
[...]
> + case C93_8X8_INTRA:
> + for (j = 0; j < 8; j++)
> + for (i = 0; i < 8; i++)
> + out[(y+j)*stride+x+i] = (buf++)[0];
bytestream_get_buffer()
> + break;
> +
> + default:
> + av_log(avctx, AV_LOG_ERROR, "unexpected type %x at %dx%d\n",
> + bt1, x, y);
> + return -1;
> + }
the switch can probably be simplified, several cases in it are quite similar
[...]
> +typedef struct {
> + voc_dec_context_t voc;
> +
> + C93BlockRecord block_records[512];
> + int current_block;
> +
> + uint32_t frame_offsets[32];
> + int current_frame;
> + int decode_audio;
please give this one a better name or comment which clarifies that this is
the frame number of the next audio packet or whatever it is exactly
> +
> + AVStream *audio;
> +} C93DemuxContext;
> +
> +static int c93_probe(AVProbeData *p)
> +{
> + if (p->buf_size < 13)
> + return 0;
> +
> + if (p->buf[0] == 0x01 && p->buf[1] == 0x00 &&
> + p->buf[4] == 0x01 + p->buf[2] &&
> + p->buf[8] == p->buf[4] + p->buf[6] &&
> + p->buf[12] == p->buf[8] + p->buf[10])
> + return AVPROBE_SCORE_MAX / 2;
you can return AVPROBE_SCORE_MAX this check looks fairly foolproof, if it
missdetects something it can always be decreased
[...]
> + uint16_t videosize;
> + uint16_t palettesize;
> + uint16_t soundsize;
why not int?
[...]
> + location = url_ftell(pb);
> + if (br->index * 2048 + c93->current_frame * 4 > location) {
> + c93->current_frame = 0;
> + c93->decode_audio = -1;
> + while (c93->current_block > 0 && br->index * 2048 > location) {
> + c93->current_block--;
> + br--;
> + }
> + }
what does this do?
[...]
> + url_fskip(pb, -(palettesize + videosize + 2));
interresting way to seek back, normally url_fseek() is used but your variant
is correct too, actually its the same in some sense ...
[...]
> + pkt->size = palettesize + videosize + 1;
this should be unneeded
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070402/042562bc/attachment.pgp>
More information about the ffmpeg-devel
mailing list