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

[Nick Renieris]

Thanks for the review Moritz, pushed fixes.

"outputted" is a word actually :)
https://forum.wordreference.com/threads/is-outputted-a-word.2707379

Στις Πέμ, 25 Ιουλ 2019 στις 4:57 μ.μ., ο/η Moritz Barsnick
<barsnick at gmx.net> έγραψε:
>
> 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
> _______________________________________________
> 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".