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

[Reimar Döffinger]

On 26.07.2019, at 09:34, Nick Renieris <velocityra at gmail.com> wrote:

> Στις Παρ, 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.

Huh? Are you thinking of templates? always_inline can usually be used the same way.
I haven't looked into the how or anything, it was just a comment in principle.

> I believe branch prediction and instruction pipelining will hide this
> delay. I doubt it has any measurable effect on performance.

There are CPUs without that.

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

I don't see the connection? The point is as input and output are 16 bit this calculation can be done as integer operations without the need for floating point and all the conversion.
Depending on required precision with either 32 or 64 bit intermediate values.
Essentially by simply changing
(value * scale_factor) * 0xFFFF
to something along the lines of 
(value * (unsigned)(scale_factor * 0xFFFF * (1<<8))) >> 8
With of course most all but one multiply and shift outside the loop.
Of course would need to look at what the actual requirements are concerning precision, rounding and range. But that should be done anyway.

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

Well, I did not find it obvious where src and dst are from and what format they are in.
Essentially if they are decoder output it's likely fine, if they are read from a file without decoding step it's likely wrong.

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

Just provide as metadata and leave to e.g. libavfilter.

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

Slow and unoptimized and in it's current form hard to SIMD optimize, especially without changing output as well though (in addition to the large bit width of floats reducing the benefit, denormals can be an issue for SIMD-accelerating float code).
Unless I miss a reason why nobody would ever want this to be faster?

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

The second comment was the more relevant one actually.
I only really care about finding some way to make this part a whole lot more readable.