On 26.07.2019, at 09:34, Nick Renieris <velocit...@gmail.com> wrote:
> Στις Παρ, 26 Ιουλ 2019 στις 2:21 π.μ., ο/η Reimar Döffinger
> <reimar.doeffin...@gmx.de> έγραψε:
>>
>> On 25.07.2019, at 17:35, velocit...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
