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

[Paul B Mahol]

On Sun, Jul 28, 2019 at 12:30 AM Reimar Döffinger <Reimar.Doeffinger at gmx.de>
wrote:

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

That does not follow DNG specification.
I really do not have time to comment on other irrelevant stuff you pointed
in your review.


>
> > 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".