[FFmpeg-devel] [PATCH v7 1/2] libavcodec/pgxdec: Add PGX decoder

[Nicolas George]

Michael Niedermayer (12020-07-04):
> that would make the code less readable IMHO

In this particular very simple case maybe.

> the same is true for the use of a macro for write_frame_*
> it was IMHO more readable before the macro

Yes, each copy was slightly more readable than the merged macro.

But now, bugs and enhancements have to be done only once.

I say: small cost, bigger benefit.

> now i dont want to confuse a GSOC student with such contraditionary
> suggestions on a patch review so if you disagree i think we should
> discuss this outside this review and tell Gautam only once we
> agree what to do.

I do not think that calm discussion cause the risk to confuse, and the
student deserves a say in this.

> There are over 2000 occurances of "== 0" in the codebase and for a byte/char
> i see nothing wrong with "== 0", its more a question of what the author
> prefers and what looks more consistent in the specific case
> 
> for a true/false variable the if (!something) is more natural than == 0
> but for something ultimately being nummeric the !byte feels a tiny bit
> less natural to me

I agree with this.

> > > +    if (bytestream2_get_bytes_left(&g) < width * height * (bpp >> 3))
> > The multiplication can overflow.
> ff_set_dimensions() should prevent this

No, it should not, because it is not aware of the specifics. The default
value for max_pixels is INT_MAX, but it is only a default value,
applications or users can set it to a higher value. More importantly,
ff_set_dimensions() does not know about bpp >> 3, doesn't know that the
whole frame needs to fit in the size of AVPacket.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200704/52b5a226/attachment.sig>