Escape 124 (RPL) decoder rev2

Per subject, revision of Escape 124 decoder per review
comments.
Biggest change is that the decoder now puts each superblock
into the
frame as it is decoded.  Also added some checks to make sure
the code
doesn't read past the end of the buffer on invalid data, and
a few
other minor changes.

-Eli

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Fri, Mar 28, 2008 at 02:55:17PM -0700, Eli Friedman
wrote:
> Per subject, revision of Escape 124 decoder per review
comments.
> Biggest change is that the decoder now puts each
superblock into the
> frame as it is decoded.  Also added some checks to make
sure the code
> doesn't read past the end of the buffer on invalid
data, and a few
> other minor changes.
> 
> -Eli

> Index: escape124.c
>
============================================================
=======
> --- escape124.c	(revision 0)
> +++ escape124.c	(revision 0)
[...]
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>

are all of these headers really needed ?


[...]
> +static CodeBook* unpack_codebook(GetBitContext* gb,
uint32_t depth,
> +                                 uint32_t length,
uint32_t alloc_length) {
> +    uint32_t i, j;

These dont need to be exactly 32 bit, so unsigned (int)
seems more appropriate.


> +    CodeBook* cb;
> +
> +    if (alloc_length >= (INT_MAX -
sizeof(CodeBook)) / sizeof(MacroBlock))
> +        return NULL;
> +    cb = av_malloc(alloc_length * sizeof(MacroBlock) +
sizeof(CodeBook));
> +    if (!cb)
> +        return NULL;
> +
> +    if (!can_safely_read(gb, length * 34))
> +        return NULL;

memleak


[...]
> +static unsigned vlc_decode(GetBitContext* gb) {
> +    unsigned value = can_safely_read(gb, 1) &&
get_bits1(gb);
> +    if (!value || !can_safely_read(gb, 3))
> +        return value;
> +
> +    value += get_bits(gb, 3);
> +    if (value != (1 + ((1 << 3) - 1)) ||
!can_safely_read(gb, 7))
> +        return value;
> +
> +    value += get_bits(gb, 7);
> +    if (value != (1 + ((1 << 3) - 1)) + ((1
<< 7) - 1) ||
> +        !can_safely_read(gb, 12))
> +        return value;
> +
> +    return value + get_bits(gb, 12);
> +}

The first check is enough as there are at least
8*FF_INPUT_BUFFER_PADDING_SIZE
bits "overallocated" at the end.
And several other checks in other functions are similarly
redundant.


> +
> +static SuperBlock read_superblock_from_buffer(
> +        uint16_t* frame_data, uint32_t stride,
> +        uint32_t superblock_row_index, uint32_t
superblock_col_index) {
> +    unsigned j, k;
> +    SuperBlock sb;

> +    frame_data += stride * 8 * superblock_row_index +
> +                  8 * superblock_col_index;

vertical align:
frame_data += stride * 8 * superblock_row_index +
                       8 * superblock_col_index;


> +    for (j = 0; j < 4; j++) {
> +        for (k = 0; k < 4; k++) {
> +            MacroBlock* curBlock = &sb.blocks[j*4
+ k];
> +            uint16_t* block_base = frame_data + j * 2
* stride + k * 2;
> +            curBlock->pixels[0] = block_base[0];
> +            curBlock->pixels[1] = block_base[1];
> +            curBlock->pixels[2] =
block_base[stride];
> +            curBlock->pixels[3] = block_base[stride
+ 1];
> +        }
> +    }
> +    return sb;
> +}
> +
> +static void write_superblock_to_buffer(
> +        uint16_t* frame_data, uint32_t stride,
> +        uint32_t superblock_row_index, uint32_t
superblock_col_index,
> +        SuperBlock sb) {
> +    unsigned j, k;
> +    frame_data += stride * 8 * superblock_row_index +
> +                  8 * superblock_col_index;
> +    for (j = 0; j < 4; j++) {
> +        for (k = 0; k < 4; k++) {
> +            MacroBlock* curBlock = &sb.blocks[j*4
+ k];
> +            uint16_t* block_base = frame_data + j * 2
* stride + k * 2;
> +            block_base[0] = curBlock->pixels[0];
> +            block_base[1] = curBlock->pixels[1];
> +            block_base[stride] =
curBlock->pixels[2];
> +            block_base[stride + 1] =
curBlock->pixels[3];
> +        }
> +    }
> +}

These 2 functions really should just be: (its simpler and
faster)
for(y=0; y<8; y++)
    memcpy(dst + y*dst_stride, src + y*src_stride,
sizeof(uint16_t)*8);


[...]

> +    if (64 + get_bits_count(&gb) >
gb.size_in_bits)
> +        return -1;

can_safely_read()


[...]
> +    if (frame_flags & (1 << 17)) {
> +        // This is the most basic codebook:
pow(2,depth) entries for 
> +        // a depth-length key
> +        if (!can_safely_read(&gb, 4))
> +            return -1;
> +        cb_depth = get_bits(&gb, 4);
> +        cb_size = (1 << cb_depth);
> +        av_free(s->codebooks[1]);
> +        s->codebooks[1] = unpack_codebook(&gb,
cb_depth, cb_size, cb_size);
> +    }
> +
> +    if (frame_flags & (1 << 18)) {
> +        // This codebook varies per superblock
> +        // FIXME: I'm not sure if I'm handling the
cb_depth=0 case
> +        // correctly
> +        if (!can_safely_read(&gb, 4))
> +            return -1;
> +        cb_depth = get_bits(&gb, 4);
> +        cb_size = (1 << cb_depth) *
s->num_superblocks;
> +        av_free(s->codebooks[0]);
> +        s->codebooks[0] = unpack_codebook(&gb,
cb_depth, cb_size, cb_size);
> +    }
> +
> +    if (frame_flags & (1 << 19)) {
> +        // This codebook can be cut off at places
other than powers of 2,
> +        // leaving some of the entries undefined.
> +        if (!can_safely_read(&gb, 4))
> +            return -1;
> +        cb_size = get_bits_long(&gb, 20);
> +        cb_depth = av_log2(cb_size - 1) + 1;
> +        cb_alloc_size = 1 << cb_depth;
> +        av_free(s->codebooks[2]);
> +        s->codebooks[2] = unpack_codebook(&gb,
cb_depth, cb_size, cb_alloc_size);
> +    }

these 3 should be in a loop instead of duplicating most code
twice


> +
> +    if (!s->codebooks[0] || !s->codebooks[1] ||
!s->codebooks[2])
> +        return -1;
> +
> +    cb_index = 0;
> +    superblock_index = 0;
> +    superblock_row_index = 0;
> +    superblock_col_index = 0;
> +

> +    do {
> +        uint32_t multi_mask = 0;
> +        uint32_t skip = vlc_decode(&gb);
> +    

trailing whitespace


[...]

> +AVCodec escape124_decoder = {
> +    "escape124",
> +    CODEC_TYPE_VIDEO,
> +    CODEC_ID_ESCAPE124,
> +    sizeof(Escape124Context),
> +    escape124_decode_init,
> +    NULL,
> +    NULL,
> +    escape124_decode_frame,
> +    CODEC_CAP_DR1,
> +};

You are forgetting to deallocate all the stuff when the
decoder is freed.


[...]
-- 
Michael     GnuPG fingerprint:
9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the
same ideas make
their appearance in the world. -- Aristotle

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Fri, Mar 28, 2008 at 4:11 PM, Michael Niedermayer
<michaelnigmx.at> wrote:
>  [...]
>  > +static CodeBook* unpack_codebook(GetBitContext*
gb, uint32_t depth,
>  > +                                 uint32_t
length, uint32_t alloc_length) {
>  > +    uint32_t i, j;
>
>  These dont need to be exactly 32 bit, so unsigned
(int) seems more appropriate.

They need to be more than 16 bits, so unsigned isn't
appropriate.

-Eli
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Fri, Mar 28, 2008 at 04:57:42PM -0700, Eli Friedman
wrote:
> On Fri, Mar 28, 2008 at 4:11 PM, Michael Niedermayer
<michaelnigmx.at> wrote:
> >  [...]
> >  > +static CodeBook*
unpack_codebook(GetBitContext* gb, uint32_t depth,
> >  > +                                 uint32_t
length, uint32_t alloc_length) {
> >  > +    uint32_t i, j;
> >
> >  These dont need to be exactly 32 bit, so unsigned
(int) seems more appropriate.
> 
> They need to be more than 16 bits, so unsigned isn't
appropriate.

Quoting POSIX:


Maximum value of type unsigned.
[CX] Minimum Acceptable Value: 4 294 967 295
------------

Now if you want your code to run on pre POSIX 16bit systems,
you could try
uint_fast32_t, though ffmpeg itself will not run on such
systems.

[...]
-- 
Michael     GnuPG fingerprint:
9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there
is freedom there
will be no State. -- Vladimir Lenin

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

"Eli Friedman" <eli.friedmangmail.com> writes:

> On Fri, Mar 28, 2008 at 4:11 PM, Michael Niedermayer
<michaelnigmx.at> wrote:
>>  [...]
>>  > +static CodeBook*
unpack_codebook(GetBitContext* gb, uint32_t depth,
>>  > +                                 uint32_t
length, uint32_t alloc_length) {
>>  > +    uint32_t i, j;
>>
>>  These dont need to be exactly 32 bit, so unsigned
(int) seems more
>>  appropriate.
>
> They need to be more than 16 bits, so unsigned isn't
appropriate.

What is that supposed to mean?  If you are seriously worried
that
someone might try to use this code on a 16-bit system (not
that it
will have the slightest chance of working), use unsigned
long.  The
required width of the type is irrelevant to the
applicability of
the unsigned type specifier.

-- 
Måns Rullgård
mansmansr.com
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

Michael Niedermayer <michaelnigmx.at> writes:

> On Fri, Mar 28, 2008 at 04:57:42PM -0700, Eli Friedman
wrote:
>> On Fri, Mar 28, 2008 at 4:11 PM, Michael
Niedermayer <michaelnigmx.at> wrote:
>> >  [...]
>> >  > +static CodeBook*
unpack_codebook(GetBitContext* gb, uint32_t depth,
>> >  > +                                
uint32_t length, uint32_t alloc_length) {
>> >  > +    uint32_t i, j;
>> >
>> >  These dont need to be exactly 32 bit, so
unsigned (int) seems more appropriate.
>> 
>> They need to be more than 16 bits, so unsigned
isn't appropriate.
>
> Quoting POSIX:
> 
>
> Maximum value of type unsigned.
> [CX] Minimum Acceptable Value: 4 294 967 295
> ------------
>
> Now if you want your code to run on pre POSIX 16bit
systems, you could try

That is not only pre-, but also current non-POSIX systems. 
C99
requires a minimum of only 16 bits for int.  It still won't
work, of
course.

-- 
Måns Rullgård
mansmansr.com
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Sat, Mar 29, 2008 at 12:49:17AM +0000, Måns Rullgård
wrote:
> Michael Niedermayer <michaelnigmx.at> writes:
> 
> > On Fri, Mar 28, 2008 at 04:57:42PM -0700, Eli
Friedman wrote:
> >> On Fri, Mar 28, 2008 at 4:11 PM, Michael
Niedermayer <michaelnigmx.at> wrote:
> >> >  [...]
> >> >  > +static CodeBook*
unpack_codebook(GetBitContext* gb, uint32_t depth,
> >> >  > +                                
uint32_t length, uint32_t alloc_length) {
> >> >  > +    uint32_t i, j;
> >> >
> >> >  These dont need to be exactly 32 bit, so
unsigned (int) seems more appropriate.
> >> 
> >> They need to be more than 16 bits, so unsigned
isn't appropriate.
> >
> > Quoting POSIX:
> > 
> >
> > Maximum value of type unsigned.
> > [CX] Minimum Acceptable Value: 4 294 967 295
> > ------------
> >
> > Now if you want your code to run on pre POSIX
16bit systems, you could try
> 
> That is not only pre-, but also current non-POSIX
systems.  C99
> requires a minimum of only 16 bits for int.  It still
won't work, of
> course.

Which 16bit system was designed after above was added to
POSIX? ;)
And yes ill try to be more precisse with what i say, i
really seem to
have lost some precission over time :(
The correct thing to say probably is something like:

Systems which conform to IEEE Std 1003.1-2001/Cor 2-2004
have at least a
UINT_MAX of 4 294 967 295, systems not conforming to above
may have lower
values. Ffmpeg will not function correctly on systems having
lower UINT_MAX.
Systems conforming to ISO/ANSI C will have at least a
UINT_MAX of 65535.


[...]
-- 
Michael     GnuPG fingerprint:
9FF2128B147EF6730BADF133611EC787040B0FAB

It is not what we do, but why we do it that matters.

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

Michael Niedermayer <michaelnigmx.at> writes:

> On Sat, Mar 29, 2008 at 12:49:17AM +0000, Måns Rullgård
wrote:
>> Michael Niedermayer <michaelnigmx.at> writes:
>> 
>> > On Fri, Mar 28, 2008 at 04:57:42PM -0700, Eli
Friedman wrote:
>> >> On Fri, Mar 28, 2008 at 4:11 PM, Michael
Niedermayer <michaelnigmx.at> wrote:
>> >> >  [...]
>> >> >  > +static CodeBook*
unpack_codebook(GetBitContext* gb, uint32_t depth,
>> >> >  > +                              
  uint32_t length, uint32_t alloc_length) {
>> >> >  > +    uint32_t i, j;
>> >> >
>> >> >  These dont need to be exactly 32
bit, so unsigned (int) seems more appropriate.
>> >> 
>> >> They need to be more than 16 bits, so
unsigned isn't appropriate.
>> >
>> > Quoting POSIX:
>> > 
>> >
>> > Maximum value of type unsigned.
>> > [CX] Minimum Acceptable Value: 4 294 967 295
>> > ------------
>> >
>> > Now if you want your code to run on pre POSIX
16bit systems, you
>> > could try
>> 
>> That is not only pre-, but also current non-POSIX
systems.  C99
>> requires a minimum of only 16 bits for int.  It
still won't work, of
>> course.
>
> Which 16bit system was designed after above was added
to POSIX? ;)

8-bit and 16-bit microcontrollers are being sold by the
million.  The
software they run obviously doesn't claim POSIX conformance
in any
way, but that's irrelevant in the context.

> And yes ill try to be more precisse with what i say, i
really seem to
> have lost some precission over time :(
> The correct thing to say probably is something like:
>
> Systems which conform to IEEE Std 1003.1-2001/Cor
2-2004 have at least a
> UINT_MAX of 4 294 967 295, systems not conforming to
above may have lower
> values. Ffmpeg will not function correctly on systems
having lower UINT_MAX.
> Systems conforming to ISO/ANSI C will have at least a
UINT_MAX of 65535.

Come on, we all know it's called ISO/IEC 9899:1999 

-- 
Måns Rullgård
mansmansr.com
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Fri, Mar 28, 2008 at 6:01 PM, Michael Niedermayer
<michaelnigmx.at> wrote:
>  Systems which conform to IEEE Std 1003.1-2001/Cor
2-2004 have at least a
>  UINT_MAX of 4 294 967 295, systems not conforming to
above may have lower
>  values. Ffmpeg will not function correctly on systems
having lower UINT_MAX.
>  Systems conforming to ISO/ANSI C will have at least a
UINT_MAX of 65535.

All right, I'll assume unsigned is at least 32 bits.

-Eli
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

On Sat, Mar 29, 2008 at 01:15:21AM +0000, Måns Rullgård
wrote:
> Michael Niedermayer <michaelnigmx.at> writes:
[...]
> > Systems conforming to ISO/ANSI C will have at
least a UINT_MAX of 65535.
> 
> Come on, we all know it's called ISO/IEC 9899:1999 

I do think that it was added in an earlier revision, likely
the first one 

[...]
-- 
Michael     GnuPG fingerprint:
9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing
something just because
microsoft did it is even more stupid. If everything ms did
were stupid they
would be bankrupt already.

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-develmplayerhq.hu
https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel