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