[FFmpeg-devel] [PATCH]Fix pnm encoding with bpc set

[Christophe Gisquet]

Hi,

2014-08-24 12:13 GMT+02:00 Carl Eugen Hoyos <cehoyos at ag.or.at>:
[...]
> -        if( avctx->bits_per_raw_sample )
> +        if (   avctx->bits_per_raw_sample
> +            && av_pix_fmt_desc_get(avctx->pix_fmt)->comp[0].depth_minus1 > 7)
>              maxdepth = (1 << avctx->bits_per_raw_sample) - 1;

So bits_per_raw_sample can be != 0 but kind of wrong in some 8 bits
cases? Do you have an example for this?

+        int bps_delta =
av_pix_fmt_desc_get(avctx->pix_fmt)->comp[0].depth_minus1 + 1 -
avctx->bits_per_raw_sample;
+        if (!avctx->bits_per_raw_sample || !bps_delta) {

Could this be moved out the loop (the compiler should do it, but...) ?
Second point, can this get negative? I don't think so because it means
something would be seriously wrong.

Otherwise, maybe you could do something like:
int bps_delta = 0;
if ( avctx->bits_per_raw_sample > 0 ) {
   bps_delta = av_pix_fmt_desc_get(avctx->pix_fmt)->comp[0].depth_minus1
+ 1 - avctx->bits_per_raw_sample;
   if (bps_delta < 0) bps_delta = 0; // maybe unneeded or wrong
}
[...]
if (!bps_delta)

+            for (j = 0; j < avctx->width; j++)
+                ((uint16_t *)bytestream)[j] = ((uint16_t *)ptr)[j] >>
bps_delta;

Is this correct? I mean, is your test case improved? If it is the
case, I think "[PATCH 2/5] pnmenc: use bits_per_raw_sample" is wrong
then, or there's maybe an endianess issue somewhere. I think during my
tests I saw something similar. From:
http://netpbm.sourceforge.net/doc/pamendian.html
it says p[bgp]m is supposedly bigendian, though at this point anything
can happen.

Otherwise, this shift would undo scaling performed elsewhere or
actually remove bits from the actual value, which would be incorrect.

Best regards,
-- 
Christophe