[FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images

[Reimar Döffinger]

On 25.07.2019, at 17:35, velocityra at gmail.com wrote:

> +    // Lookup table lookup
> +    if (lut)
> +        value = lut[value];

As this function is in the innermost loop, doing the if here instead of having 2 different implementations is likely not ideal speed-wise.

> +    // Color scaling
> +    value = av_clip_uint16_c((unsigned)(((float)value * scale_factor) * 0xFFFF));

As input and output are both 16 bit I wonder if floating-point isn't rather overkill compared to doing fixed-point arithmetic.

> 
> +    if (is_u16) {
> +        for (line = 0; line < height; line++) {
> +            uint16_t *dst_u16 = (uint16_t *)dst;
> +            uint16_t *src_u16 = (uint16_t *)src;
> +
> +            for (col = 0; col < width; col++)
> +                *dst_u16++ = dng_raw_to_linear16(*src_u16++, s->dng_lut, s->black_level, scale_factor);
> +
> +            dst += dst_stride * sizeof(uint16_t);
> +            src += src_stride * sizeof(uint16_t);

Is all this casting working correctly on e.g. big-endian?
Also using sizeof on uint16_t and uint8_t seems a bit overkill.
Also not sure if since these are essentially brightness/contrast adjustments if we should't rather just have a way to export the transform to use...


> @@ -1519,6 +1773,26 @@ again:
>         return AVERROR_INVALIDDATA;
>     }
> 
> +    /* Handle DNG images with JPEG-compressed tiles */
> +
> +    if (s->tiff_type == TIFF_TYPE_DNG || s->tiff_type == TIFF_TYPE_CINEMADNG) {
> +        if (s->is_jpeg) {
> +            if (s->is_bayer) {
> +                if ((ret = dng_decode(avctx, (AVFrame*)data, avpkt)) > 0)
> +                    *got_frame = 1;
> +                return ret;
> +            } else {
> +                avpriv_report_missing_feature(avctx, "DNG JPG-compressed non-bayer-encoded images");
> +                return AVERROR_PATCHWELCOME;
> +            }
> +        } else if (s->is_tiled) {
> +            avpriv_report_missing_feature(avctx, "DNG uncompressed tiled images");
> +            return AVERROR_PATCHWELCOME;
> +        }

There is no need for an "else" block if the "if" block ends in a return.
Also putting the error handling first/at the deepest indentation level results in more readable code generally.