[Ffmpeg-devel] [PATCH] C93 demuxer and decoder (GSoC qualification task)
Anssi Hannula
anssi.hannula
Mon Apr 2 15:46:55 CEST 2007
Michael Niedermayer wrote:
> Hi
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
Indeed, the division and modulo don't need to be done for every pixel.
> also the prev_y += size; looks odd are you sure this is correct?
If an overflow happens on the right border, the picture data is copied
from the beginning (left), one line of blocks (8x8 or 4x4) down, hence
+= size.
I'm not 100% sure, though, as the wiki page didn't talk about overflows.
Some of the sample c93 files do have them, and handling them this way
seemed most logical (to me anyway), and the files seem to render properly.
Thinking about it, I guess it is possible the data should be copied from
the beginning of the same block line where the overflow happened
instead. There are only few cases where overflow happens, so I'll have
to check this by doing some careful inspection on which one renders better.
[...]
>
>> + 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
OK.
> iam not sure if the double reget_buffer() is completely safe, normally
> id just call it on the frame which is changed and output ...
The intention is to write the new frame on top of the frame from two
frames ago, as explained in the wiki:
Wiki note about C93_4X4_FROM_CURR = 0x07
"Note, that due to doublebuffering this can reuse of blocks painted two
frames ago as well as already processed blocks of the current frame."
Wiki note about C93_NOOP = 0x0E
"Note that, due to the double buffering scheme used in the original
decoder, this has the implicit effect of rendering the 8x8 block decoded
two frames ago."
Is there some easier or more correct way to do that instead of two
reget_buffers?
> 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()
Ok.
>> + 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()
Ok.
>
>> + }
>
> changing the old picture is not safe it could be just drawn to the screen
> by the user application in another thread
So I should instead memcpy the palette from oldpic to newpic, to ensure
the new palette gets applied in the other one of our AVFrames as well?
>
>> + }
>> +
>> + 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 ?
Hmm.. no reason. Will change.
>
>> + if (!bt1) {
>> + bt1 = buf[0] & 0x0F;
>
>> + bt2 = (buf[0] >> 4) & 0x0F;
>
> the &0x0F is useless
Indeed.
>
>> + buf++;
>> + }
>
> besides
> if(!bt)
> bt=*buf++;
> switch(bt & 0xF)
> ...
> bt>>=4;
>
> will work too and be simpler
True, I will change that.
>
>> +
>> + switch (bt1) {
>> + case C93_8X8_FROM_PREV:
>> + offset = AV_RL16(buf);
>> + buf += 2;
>
> bytestream_get_le16()
Ok.
>> + 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
Didn't know that one, thanks.
>
>> + out[(y+j)*stride+x+i] = oldpic->data[0][loc];
>
> cant memcpy() be used here instead of 1 pixel at a time copying?
I'm not sure. The source data could be non-contiguous in the case of
overflow about which I talked in the beginning of message.
>
> [...]
>> + 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?
A good point.
>
> [...]
>> + 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
Okay.
>
> [...]
>> + 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()
Yep, will change.
>
>> + 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
>
Yes, I'll look into it.
> [...]
>
>> +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
Right.
>
>> +
>> + 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
Okay, will change.
>
> [...]
>> + uint16_t videosize;
>> + uint16_t palettesize;
>> + uint16_t soundsize;
>
> why not int?
The data in the file is uint16_t. But true, int should be preferred here.
>
> [...]
>> + 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?
If the file was seeked backwards (by user), this would cause
current_block to be updated appropriately instead of just seeking
forward back to where we were. But as seeking is not implemented anyway,
I guess this can be removed...
>
> [...]
>> + 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 ...
Also see my reply to Reimar for possible solution to remove this seek.
>
> [...]
>> + pkt->size = palettesize + videosize + 1;
>
> this should be unneeded
OK.
Thanks to everybody for the comments :)
--
Anssi Hannula
More information about the ffmpeg-devel
mailing list