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

[Nick Renieris]

Στις Παρ, 26 Ιουλ 2019 στις 2:21 π.μ., ο/η Reimar Döffinger
<Reimar.Doeffinger at gmx.de> έγραψε:
>
> 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.

If this were C++ this'd be trivial, but as it stands I don't see a way
to do this without sacrificing code readability and/or size.
I believe branch prediction and instruction pipelining will hide this
delay. I doubt it has any measurable effect on performance.

> > +    // 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.

Scaling in the [0.0, 1.0] range is mentioned in the DNG spec and it's
also what dcraw does.

> >
> > +    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?

Not sure, I don't see why not, considering I've seen such casting in
other parts of ffmpeg.

> Also using sizeof on uint16_t and uint8_t seems a bit overkill.

It makes the programmer's intention clear, I think that's worth the
few extra characters.

> 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...

Export to where? I don't see why you'd need to complicate this by
calling into other components, the transformation code is concise,
clear and accurate for this use case.

>
> > @@ -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.

Indeed, but in my opinion it makes the code clearer on first glance
(if you miss the return). I can change it if you insist.

> Also putting the error handling first/at the deepest indentation level results in more readable code generally.

That's true, I will do that.