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
<barsn...@gmx.net> έγραψε:
>
> On Thu, Jul 25, 2019 at 15:12:53 +0300, velocit...@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
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".
