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 <one...@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
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
