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

[Moritz Barsnick]

On Thu, Jul 25, 2019 at 15:12:53 +0300, velocityra at gmail.com wrote:

Nit:

>  tiff_decoder_suggest="zlib lzma"
>  tiff_encoder_suggest="zlib"
> +tiff_decoder_select="mjpeg_decoder"
>  truehd_decoder_select="mlp_parser"

You should pair the new decoder line with the other decoder line, not
place it below the encoder.

> +    int is_jpeg; // 0 - Not JPEG, 1 - JPEG, 2 - New JPEG

"is" makes this sound boolean, perhaps better "jpeg_type" or something
like that.

OTOH, I can't find the differentiation between 1 and 2 used anywhere.

> +    // Lookup table lookup
> +    if (lut) value = lut[value];

You probably need to break the line, according to ffmpeg coding rules.

> +    float scale_factor;
> +
> +    scale_factor = 1.0 / (s->white_level - s->black_level);

This is promoting the floating point operation to a double precision
operation (significant for some platforms). Either use double variables
in the first place - if the extra precision can be justified - or use
the "f" suffix to the constant: "1.0f".

> +    ret = avcodec_receive_frame(s->avctx_mjpeg, s->jpgframe);
> +    if (ret < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "JPEG decoding error (%d).\n", ret);

I believe the return value can be decoded into a string, e.g. with
av_err2str().

> +    /* Copy the outputted tile's pixels from 'jpgframe' to to 'frame' (final buffer */

Outputted is not a word. ;-) Don't worry about that, but there's a
duplicate "to" in there, and the bracket is not closed.

> +        uint32_t lut_offset = value;
> +        uint32_t lut_size = count;
> +        uint32_t lut_wanted_size = 1 << s->bpp;
> +        if (lut_wanted_size != lut_size)
> +            av_log(s->avctx, AV_LOG_WARNING, "DNG contains LUT with invalid size (%d), disabling LUT\n", lut_size);
> +        else if (lut_offset >= bytestream2_size(&s->gb))
> +            av_log(s->avctx, AV_LOG_WARNING, "DNG contains LUT with invalid offset (%d), disabling LUT\n", lut_offset);

Nit: the proper format identifier for uint32_t is '"%" PRIu32'. (

> +            } else {
> +                av_log(avctx, AV_LOG_ERROR, "DNG JPG-compressed non-bayer-encoded images are not supported\n");
> +                return AVERROR_PATCHWELCOME;

Alternatively (to av_log()) use avpriv_report_missing_feature().

> +            }
> +        } else if (s->is_tiled) {
> +            av_log(avctx, AV_LOG_ERROR, "DNG uncompressed tiled images are not supported\n");
> +            return AVERROR_PATCHWELCOME;

Ditto.

> +    s->jpgframe = av_frame_alloc();
> +    if (!s->jpgframe)
> +        return AVERROR(ENOMEM);
> +
> +    /* Prepare everything needed for JPEG decoding */
> +    codec = avcodec_find_decoder(AV_CODEC_ID_MJPEG);
> +    if (!codec)
> +        return AVERROR_BUG;
> +    s->avctx_mjpeg = avcodec_alloc_context3(codec);
> +    if (!s->avctx_mjpeg)
> +        return AVERROR(ENOMEM);

Don't you need to free s->jpgframe here? (And codec?)

Cheers,
Moritz