[FFmpeg-devel] [PATCH 2/2] avcodec: add WinCAM Motion Video decoder

Michael Niedermayer michael at niedermayer.cc
Tue Aug 28 03:40:42 EEST 2018


Hi

On Mon, Aug 27, 2018 at 07:31:21PM +0100, Rostislav Pehlivanov wrote:
> On Sat, 25 Aug 2018 at 21:12, Paul B Mahol <onemda at gmail.com> wrote:
[...]
> > +
> > +    blocks = bytestream2_get_le16(&gb);
> > +    if (blocks > 5) {
> > +        GetByteContext bgb;
> > +        int x = 0, size;
> > +
> > +        if (blocks * 8 >= 0xFFFF) {
> > +            size = bytestream2_get_le24(&gb);
> > +        } else if (blocks * 8 >= 0xFF) {
> > +            size = bytestream2_get_le16(&gb);
> > +        } else {
> > +            size = bytestream2_get_byte(&gb);
> > +        }
> >
> 
> No need for brackets here, they're all one line expressions.

You are correct. The extra brackets simplify any future additions to
the branches though (which was why this style was often prefered long ago)
It may not apply here, but for example what the {} do to changes is that:

        if (blocks * 8 >= 0xFFFF) {
            size = bytestream2_get_le24(&gb);
+           if (size == whatever)
+               return AVERROR_INVALIDDATA;
        } else if (blocks * 8 >= 0xFF) {
            size = bytestream2_get_le16(&gb);
        } else {
            size = bytestream2_get_byte(&gb);
        }

vs:

-       if (blocks * 8 >= 0xFFFF)
+       if (blocks * 8 >= 0xFFFF) {
            size = bytestream2_get_le24(&gb);
-       else if (blocks * 8 >= 0xFF)
+           if (size == whatever)
+               return AVERROR_INVALIDDATA;
+       } else if (blocks * 8 >= 0xFF)
            size = bytestream2_get_le16(&gb);
        else
            size = bytestream2_get_byte(&gb);

            
The first diff is easier & quicker to read, aka it helps with future
maintaince
also, extra {} often cost no vertical space

I just now realized you often recommand to remove {}, which is why i
wrote above. This is not meant as opposition for this specific change,
just wanted to explain the reasoning that iam aware of behind the use of {}
in cases that seemingly dont benefit from them.



[...]
> 
> 
> > +
> > +        bytestream2_skip(&gb, size);
> > +        bytestream2_init(&bgb, s->block_data, blocks * 8);
> > +
> > +        for (int i = 0; i < blocks; i++) {
> > +            int w, h;
> > +
> > +            bytestream2_skip(&bgb, 4);
> > +            w = bytestream2_get_le16(&bgb);
> > +            h = bytestream2_get_le16(&bgb);
> > +            x += 3 * w * h;
> > +        }
> > +

> > +        if (x >= 0xFFFF) {
> > +            bytestream2_skip(&gb, 3);
> > +        } else if (x >= 0xFF) {
> > +            bytestream2_skip(&gb, 2);
> > +        } else {
> > +            bytestream2_skip(&gb, 1);
> > +        }
> >
> 
> Same, oneliners so no need for brackets.
> 
> 
> +
> > +        skip = bytestream2_tell(&gb);
> > +
> > +        s->zstream.next_in  = avpkt->data + skip;
> > +        s->zstream.avail_in = avpkt->size - skip;
> > +
> > +        bytestream2_init(&gb, s->block_data, blocks * 8);
> > +    } else {
> > +        int x = 0;
> > +
> > +        bytestream2_seek(&gb, 2, SEEK_SET);
> > +
> > +        for (int i = 0; i < blocks; i++) {
> > +            int w, h;
> > +
> > +            bytestream2_skip(&gb, 4);
> > +            w = bytestream2_get_le16(&gb);
> > +            h = bytestream2_get_le16(&gb);
> > +            x += 3 * w * h;
> > +        }
> > +
> > +        if (x >= 0xFFFF) {
> > +            bytestream2_skip(&gb, 3);
> > +        } else if (x >= 0xFF) {
> > +            bytestream2_skip(&gb, 2);
> > +        } else {
> > +            bytestream2_skip(&gb, 1);
> > +        }
> >
> 
> Same.
> 
> 
> 
> > +
> > +        skip = bytestream2_tell(&gb);
> > +
> > +        s->zstream.next_in  = avpkt->data + skip;
> > +        s->zstream.avail_in = avpkt->size - skip;

This code is identical, and could maybe be factored out

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180828/60e6ee8f/attachment.sig>


More information about the ffmpeg-devel mailing list