[Ffmpeg-devel] Re: [PATCH] Tiertex .SEQ files support

Gregory Montoir cyx
Sun Sep 24 22:59:10 CEST 2006


Hi,

Updated patches.

Michael Niedermayer wrote:
> Hi
> 
> On Wed, Sep 20, 2006 at 12:19:48AM +0200, Gregory Montoir wrote:
>> Hi,
>>
>> Updated patch.
>>
>> Michael Niedermayer wrote:
>>> Hi
>>>
> [...]
>>
>> I also have a question regarding the way palette update is handled.
>> Looking at idcin.c/idcinvideo.c, AVPaletteControl is used. If I
>> understand things correctly, the AVCodecContext structure contains
>> this field, the demuxer updates it when it detects a palette change
>> and next time the video decoder processes a frame, it can handle
>> it properly. But what happens if the (demuxer)_read_packet() and
>> (video)_decode_frame calls are not exactly sequential ? For example,
>> in the following situation :
>>
>> demuxer_read_packet() is called
>>  -> a palette change is detected on frame (n+0), AVPaletteControl
>>     is updated
>>
>> demuxer_read_packet() is called (probably to buffer data)
>>  -> no palette change in frame (n+1)
>>
>> demuxer_read_packet() is called a third time (probably to buffer even
>> more data)
>>  -> a palette change is detected on frame (n+2), AVPaletteControl
>>     is updated
>>
>> video_decode_frame() is called
>>  -> it processes frame (n+0), but, with the palette of frame (n+2) (as
>>     the demuxer has just updated AVPaletteControl in the previous call).
>>
>> I am just wondering because that's the behaviour I'm observing under
>> ffplay. Is there a way to do, properly, some kind of synchronization ?
> 
> no, the AVPaletteControl system is totaly broken, a patch which removes
> it and instead passes palette changes as AVPackets would be very welcome

Noted.


>> +static unsigned char *seq_unpack_rle_frame(unsigned char *src, unsigned char *dst, int dst_size)
>> +{
>> +    int i, len, sz;
>> +    GetBitContext gb;
>> +    int code_table[64];
>> +
>> +    for (i = 0, sz = 0; i < 64 && sz < dst_size;) {
>> +        init_get_bits(&gb, src, 8); ++src;
>> +
>> +        code_table[i] = get_sbits(&gb, 4);
>> +        sz += ABS(code_table[i]);
>> +        ++i;
>> +
>> +        code_table[i] = get_sbits(&gb, 4);
>> +        sz += ABS(code_table[i]);
>> +        ++i;
>> +    }
> 
> calling init_get_bits() in the loop is of course not what i had in mind,
> i rather meant that you should do a single init_get_bits() per frame
> though i see now that there are some problems with that approch
> as theres are plenty of cases where byte based accesses happen
> 
> in the specific case above i would suggest to either
> A. put the init_get_bits before the loop
> B. do the init_get_bits outside and long before the function and use
>    get_bits_count() to find the byte pos for the memcpy/memset
> C. forget init_get_bits() and return to the old byte based code

I think using get_bits() for this appropriate so I just moved the init
function call before the for loop like suggestion 'A'. As there can't be
more than 64 bytes to encode a 8x8 block, init_get_bits() can be called
with this value as a "maximum bitstream size".


> [...]
>> +static unsigned char *seq_decode_op2(SeqVideoContext *seq, unsigned char *src, unsigned char *dst)
>> +{
>> +    int b, i, len;
>> +    GetBitContext gb;
>> +
>> +    len = *src++;
>> +    if (len & 0x80) {
>> +        switch (len & 15) {
>> +        case 1:
>> +            src = seq_unpack_rle_frame(src, seq->unpack_buffer, sizeof(seq->unpack_buffer));
>> +            for (b = 0; b < 8; ++b) {
>> +                memcpy(dst, &seq->unpack_buffer[b * 8], 8);
>> +                dst += seq->frame.linesize[0];
>> +            }
>> +            break;
>> +        case 2:
>> +            src = seq_unpack_rle_frame(src, seq->unpack_buffer, sizeof(seq->unpack_buffer));
>> +            for (i = 0; i < 8; ++i) {
>> +                for (b = 0; b < 8; ++b)
>> +                    dst[b * seq->frame.linesize[0]] = seq->unpack_buffer[i * 8 + b];
>> +                ++dst;
>> +            }
>> +            break;
>> +        }
> 
> i would move the call to seq_unpack_rle_frame before the switch so that there
> just a single call, that way the cost for inlinng the function would be
> smaller for the compiler (if it does inline it ...)

If you move that call before the switch, you should also handle the case
where (len & 3) == 0 (this probably never happens, I need to check the
sample files, but I would prefer being "compatible" with the original
playback code).


>> +    } else {
>> +        unsigned char *color_table = src;
>> +        src += len;
>> +        if (len > 8) {
>> +            init_get_bits(&gb, src, 32 * 8); src += 32;
>> +            for (b = 0; b < 8; ++b) {
>> +                for (i = 0; i < 8; ++i)
>> +                    dst[i] = color_table[get_bits(&gb, 4)];
>> +                dst += seq->frame.linesize[0];
>> +            }
>> +        } else if (len > 4) {
>> +            init_get_bits(&gb, src, 24 * 8); src += 24;
>> +            for (b = 0; b < 8; ++b) {
>> +                for (i = 0; i < 8; ++i)
>> +                    dst[i] = color_table[get_bits(&gb, 3)];
>> +                dst += seq->frame.linesize[0];
>> +            }
>> +        } else if (len > 2) {
>> +            init_get_bits(&gb, src, 16 * 8); src += 16;
>> +            for (b = 0; b < 8; ++b) {
>> +                for (i = 0; i < 8; ++i)
>> +                    dst[i] = color_table[get_bits(&gb, 2)];
>> +                dst += seq->frame.linesize[0];
>> +            }
>> +        } else {
>> +            init_get_bits(&gb, src, 8 * 8); src += 8;
>> +            for (b = 0; b < 8; ++b) {
>> +                for (i = 0; i < 8; ++i)
>> +                    dst[i] = color_table[get_bits(&gb, 1)];
>> +                dst += seq->frame.linesize[0];
>> +            }
>> +        }
>> +    }
> 
> why not: (assuming its not slow)
> 
> if(len > 8) bits= 4;
> else        bits= foobar_table[len]; (or some av_log2 based stuff)
> (init_get_bits(&gb, src, 8*8*bits); src += 8*bits)
> for (b = 0; b < 8; ++b) {
>     for (i = 0; i < 8; ++i)
>         dst[i] = color_table[get_bits(&gb, bits)];
>     dst += seq->frame.linesize[0];
> }

Looks nice, I rewrote that part using a custom log2 table (I haven't
been able to re-use the existing av_log2 table, as I need to "round up"
the result rather that rounding down).


> [...]
>> +static int seq_parse_frame_data(SeqDemuxContext *seq, ByteIOContext *pb)
>> +{
>> +    unsigned int offset, end_offset;
>> +
>> +    if (get_buffer(pb, seq->current_frame_data, SEQ_FRAME_SIZE) != SEQ_FRAME_SIZE)
>> +        return AVERROR_IO;
>> +
>> +    /* sound data */
>> +    offset = LE_16(&seq->current_frame_data[0]);
>> +    if (offset != 0) {
>> +        seq->current_audio_pkt_size = SEQ_AUDIO_BUFFER_SIZE * 2;
>> +        seq->current_audio_pkt_ptr = seq->current_frame_data + offset;
>> +    } else {
>> +        seq->current_audio_pkt_size = 0;
>> +        seq->current_audio_pkt_ptr = 0;
>> +    }
>> +
>> +    /* palette data */
>> +    offset = LE_16(&seq->current_frame_data[2]);
>> +    if (offset != 0) {
>> +        seq->current_palette_pkt_size = 768;
>> +        seq->current_palette_pkt_ptr = seq->current_frame_data + offset;
>> +    } else {
>> +        seq->current_palette_pkt_size = 0;
>> +        seq->current_palette_pkt_ptr = 0;
>> +    }
>> +
>> +    /* video data */
>> +    offset = LE_16(&seq->current_frame_data[8]);
>> +    if (offset != 0) {
>> +        end_offset = LE_16(&seq->current_frame_data[10]);
>> +        if (end_offset == 0)
>> +            end_offset = LE_16(&seq->current_frame_data[12]);
>> +            if (end_offset == 0)
>> +                end_offset = LE_16(&seq->current_frame_data[14]);
> 
> the indention looks wrong

Oops, too much Python for me recently :)


>> +        seq_fill_buffer(seq, seq->current_frame_data[5],
>> +          seq->current_frame_data + offset,
>> +          end_offset - offset);
>> +    }
>> +    offset = LE_16(&seq->current_frame_data[10]);
>> +    if (offset != 0) {
>> +        end_offset = LE_16(&seq->current_frame_data[12]);
>> +        if (end_offset == 0)
>> +            end_offset = LE_16(&seq->current_frame_data[14]);
>> +        seq_fill_buffer(seq, seq->current_frame_data[6],
>> +          seq->current_frame_data + offset,
>> +          end_offset - offset);
>> +    }
>> +    offset = LE_16(&seq->current_frame_data[12]);
>> +    if (offset != 0) {
>> +        end_offset = LE_16(&seq->current_frame_data[14]);
>> +        seq_fill_buffer(seq, seq->current_frame_data[7],
>> +          seq->current_frame_data + offset,
>> +          end_offset - offset);
>> +    }
> 
> for(i=0; i<3; i++)
>     buf_num[i]= get_byte();
> for(i=0; i<4; i++)
>     offset[i]= get_le16();
> end_offset[3]= 0;
> for(i=2; i>=0; i--)
>     end_offset[i]= offset[i+1] ? offset[i+1] : end_offset[i+1]
> 
> for(i=0; i<3; i++){
>     if(offset[i])
>         seq_fill_buffer(seq, buf_num[i],
>             seq->current_frame_data + offset[i],
>             end_offset[i] - offset[i]);
> }
> 
> dont you think that something like the above code is more readable? also 
> there are no url_fseek() needed ...

Indeed, the for loop makes the code more friendly.

But still, I don't see how you can use the get_* functions on
ByteIOContext *and not* using url_fseek later on... Looking at your
pseudo code, seq_fill_buffer takes an offset as argument. So if you want
to copy data from a given offset, without having read the whole 6kb
buffer before, I will have to "seek". Same goes for the sound and
palette data.

Am I missing something ?


> [...]
>> @@ -463,8 +463,13 @@
>>          NEG_USR32(name##_cache, num)
>>  # endif
>>
>> +# ifdef ALT_BITSTREAM_READER_LE
>>  #   define SHOW_SBITS(name, gb, num)\
>> +        NEG_SSR32((name##_cache)<<(32-(num)), num)
>> +# else
>> +#   define SHOW_SBITS(name, gb, num)\
>>          NEG_SSR32(name##_cache, num)
>> +# endif
> 
> these 2 SHOW_SBITS should be put under the ifdef ALT_BITSTREAM_READER_LE under
> which the SHOW_UBITS are too -> fewer #ifdefs 
> 
> except these things the bitstream.h patch looks ok
> 
> [...]

I reordered the #ifdefs in the updated bitstream patch.

Regards,
Gregory
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ffmpeg-tiertexseq-20060924.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20060924/5ea4e3c7/attachment.asc>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ffmpeg-bitstream-20060924.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20060924/5ea4e3c7/attachment.txt>



More information about the ffmpeg-devel mailing list