Re: [FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images
On 28.07.2019, at 16:45, Paul B Mahol wrote: > On Sun, Jul 28, 2019 at 4:39 PM Reimar Döffinger > wrote: > >> On 28.07.2019, at 08:55, Paul B Mahol wrote: >> Just provide as metadata and leave to e.g. libavfilter. >>> >>> That does not follow DNG specification. >>> I really do not have time to comment on other irrelevant stuff you >> pointed >>> in your review. >> >> If you had just told me that I should back off in a polite manner instead >> of this dismissive-insulting >> way it would have taken me much less effort to acknowledge your point. >> And over time you could save a lot of time thanks to not having to complain >> about people being biased against you. > > > Sorry if you feel attacked, but I simply do not like it. That is fine to tell me. I'll at least make an effort. Just the wording discourages from doing so. > Instead of complaining you could send patch one or two. (Assuming you mean the review by "complaining") Sorry, it was just meant to be a review, not complaining, but I know I tend to write in a way that is too much like complaining. There are quite a few patches that go unreviewed because (I assume) there are not enough reviewers as usual, so I've rather been reviewing than developing. Plus I have no personal itch to scratch to send a patch for currently. But if more people think there are more useful ways to contribute I'll certainly listen. ___ 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".
Re: [FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images
On Sun, Jul 28, 2019 at 4:39 PM Reimar Döffinger wrote: > On 28.07.2019, at 08:55, Paul B Mahol wrote: > > >> > >> Just provide as metadata and leave to e.g. libavfilter. > >> > > > > That does not follow DNG specification. > > I really do not have time to comment on other irrelevant stuff you > pointed > > in your review. > > If you had just told me that I should back off in a polite manner instead > of this dismissive-insulting > way it would have taken me much less effort to acknowledge your point. > And over time you could save a lot of time thanks to not having to complain > about people being biased against you. Sorry if you feel attacked, but I simply do not like it. Instead of complaining you could send patch one or two. ___ 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".
Re: [FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images
On 28.07.2019, at 08:55, Paul B Mahol wrote: >> >> Just provide as metadata and leave to e.g. libavfilter. >> > > That does not follow DNG specification. > I really do not have time to comment on other irrelevant stuff you pointed > in your review. If you had just told me that I should back off in a polite manner instead of this dismissive-insulting way it would have taken me much less effort to acknowledge your point. And over time you could save a lot of time thanks to not having to complain about people being biased against you. ___ 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".
Re: [FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images
On 28.07.2019, at 10:40, Nick Renieris wrote: > Actually, I checked a more accurate version of my loop, and GCC > optimizes away the LUT check anyway: > https://godbolt.org/z/G1e1R4 > As you can see it's smart enough to create 2 versions of my functions > (started at L3 with a lookup and L7 without it) and it does the check > outside. > > There's no guarantee this is happening with the actual version of > course (it could be slower, or even faster if it also optimizes it > through dng_blit). > I could check the actual disasm in FFmpeg, but I don't think it's > worth it at this point (my mentor agrees). Sorry, I did not know you already had discussions with an FFmpeg developer about the design. I tend to review in a way that I just comment on anything I feel is not optimal. I understand that much of it might not be reasonable to do differently in the end, but feel that it is often enough to make it worth discussing things. But I realize it can come across the wrong way, so sorry if I gave you a bad review experience, and thanks for considering my comments. Best regards, Reimar ___ 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".
Re: [FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images
Actually, I checked a more accurate version of my loop, and GCC optimizes away the LUT check anyway: https://godbolt.org/z/G1e1R4 As you can see it's smart enough to create 2 versions of my functions (started at L3 with a lookup and L7 without it) and it does the check outside. There's no guarantee this is happening with the actual version of course (it could be slower, or even faster if it also optimizes it through dng_blit). I could check the actual disasm in FFmpeg, but I don't think it's worth it at this point (my mentor agrees). Στις Κυρ, 28 Ιουλ 2019 στις 10:40 π.μ., ο/η Nick Renieris έγραψε: > > Στις Κυρ, 28 Ιουλ 2019 στις 1:30 π.μ., ο/η Reimar Döffinger > έγραψε: > > > > 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. > > You're totally right (I checked on godbolt just to make sure), I > hadn't considered that. Would need a trivial change (plus the function > is already inlined) so I'll do it. > > > > 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. > > Yes, but with neither? Branch prediction, sure, but I'd like to hear > one example of a CPU FFmpeg runs on without pipelining (I genuinely > don't know if there are). > > > Of course would need to look at what the actual requirements are concerning > > precision, rounding and range. But that should be done anyway. > Like I said, I did. It's not possible :) > > > 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. > > Right, I see your concern. They are read from a file and they can be > both LE/BE, but that's specified in the file and there is logic in > tiff.c to account for it. So when we process them they are guaranteed > to be the same endianness as the host. > > > >> 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. > > Not sure how I'd do that or if it'd be appropriate/feasible. > > > > 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? > > I'll take out the comparison which should make it faster, I'll talk > with my mentor if he thinks I should use libavfilter. > > > >> 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. > > Ok, hopefully the code I posted right after your review is readable enough. ___ 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".
Re: [FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images
Στις Κυρ, 28 Ιουλ 2019 στις 1:30 π.μ., ο/η Reimar Döffinger έγραψε: > > 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. You're totally right (I checked on godbolt just to make sure), I hadn't considered that. Would need a trivial change (plus the function is already inlined) so I'll do it. > > 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. Yes, but with neither? Branch prediction, sure, but I'd like to hear one example of a CPU FFmpeg runs on without pipelining (I genuinely don't know if there are). > Of course would need to look at what the actual requirements are concerning > precision, rounding and range. But that should be done anyway. Like I said, I did. It's not possible :) > 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. Right, I see your concern. They are read from a file and they can be both LE/BE, but that's specified in the file and there is logic in tiff.c to account for it. So when we process them they are guaranteed to be the same endianness as the host. > >> 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. Not sure how I'd do that or if it'd be appropriate/feasible. > > 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? I'll take out the comparison which should make it faster, I'll talk with my mentor if he thinks I should use libavfilter. > >> 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. Ok, hopefully the code I posted right after your review is readable enough. ___ 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".
Re: [FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images
On Sun, Jul 28, 2019 at 12:30 AM Reimar Döffinger
wrote:
> On 26.07.2019, at 09:34, Nick Renieris wrote:
>
> > Στις Παρ, 26 Ιουλ 2019 στις 2:21 π.μ., ο/η Reimar Döffinger
> > έγραψε:
> >>
> >> 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)
> * 0x));
> >>
> >> 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) * 0x
> to something along the lines of
> (value * (unsigned)(scale_factor * 0x * (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.
>
That does not follow DNG specification.
I really do not have time to comment on other irrelevant stuff you pointed
in your review.
>
> > 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
Re: [FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images
On 26.07.2019, at 09:34, Nick Renieris wrote:
> Στις Παρ, 26 Ιουλ 2019 στις 2:21 π.μ., ο/η Reimar Döffinger
> έγραψε:
>>
>> 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) *
>>> 0x));
>>
>> 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) * 0x
to something along the lines of
(value * (unsigned)(scale_factor * 0x * (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".
Re: [FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images
Στις Παρ, 26 Ιουλ 2019 στις 2:21 π.μ., ο/η Reimar Döffinger
έγραψε:
>
> 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) *
> > 0x));
>
> 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".
Re: [FFmpeg-devel] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images
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.
> +// Color scaling
> +value = av_clip_uint16_c((unsigned)(((float)value * scale_factor) *
> 0x));
As input and output are both 16 bit I wonder if floating-point isn't rather
overkill compared to doing fixed-point arithmetic.
>
> +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?
Also using sizeof on uint16_t and uint8_t seems a bit overkill.
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...
> @@ -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.
Also putting the error handling first/at the deepest indentation level results
in more readable code generally.
___
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] [PATCH v7 2/2] lavc/tiff: Decode embedded JPEGs in DNG images
From: Nick Renieris
Used a technique similar to lavc/tdsc.c for invoking the MJPEG decoder.
This commit adds support for:
- DNG tiles
- DNG tile huffman lossless JPEG decoding
- DNG 8-bpp ("packed" as dcraw calls it) decoding
- DNG color scaling [1]
- LinearizationTable tag
- BlackLevel tag
[1]: As specified in the DNG Specification - Chapter 5
Signed-off-by: Nick Renieris
---
configure | 1 +
libavcodec/Makefile | 2 +-
libavcodec/tiff.c | 315 +++-
libavcodec/tiff.h | 2 +
4 files changed, 312 insertions(+), 8 deletions(-)
diff --git a/configure b/configure
index 5a4f507246..6726883d5b 100755
--- a/configure
+++ b/configure
@@ -2811,6 +2811,7 @@ tdsc_decoder_deps="zlib"
tdsc_decoder_select="mjpeg_decoder"
theora_decoder_select="vp3_decoder"
thp_decoder_select="mjpeg_decoder"
+tiff_decoder_select="mjpeg_decoder"
tiff_decoder_suggest="zlib lzma"
tiff_encoder_suggest="zlib"
truehd_decoder_select="mlp_parser"
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 3cd73fbcc6..f814c69996 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -616,7 +616,7 @@ OBJS-$(CONFIG_TARGA_ENCODER) += targaenc.o rle.o
OBJS-$(CONFIG_TARGA_Y216_DECODER) += targa_y216dec.o
OBJS-$(CONFIG_TDSC_DECODER)+= tdsc.o
OBJS-$(CONFIG_TIERTEXSEQVIDEO_DECODER) += tiertexseqv.o
-OBJS-$(CONFIG_TIFF_DECODER)+= tiff.o lzw.o faxcompr.o tiff_data.o
tiff_common.o
+OBJS-$(CONFIG_TIFF_DECODER)+= tiff.o lzw.o faxcompr.o tiff_data.o
tiff_common.o mjpegdec.o
OBJS-$(CONFIG_TIFF_ENCODER)+= tiffenc.o rle.o lzwenc.o tiff_data.o
OBJS-$(CONFIG_TMV_DECODER) += tmv.o cga_data.o
OBJS-$(CONFIG_TRUEHD_DECODER) += mlpdec.o mlpdsp.o
diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
index c520d7df83..09cec2 100644
--- a/libavcodec/tiff.c
+++ b/libavcodec/tiff.c
@@ -35,6 +35,7 @@
#include "libavutil/attributes.h"
#include "libavutil/avstring.h"
+#include "libavutil/error.h"
#include "libavutil/intreadwrite.h"
#include "libavutil/imgutils.h"
#include "libavutil/opt.h"
@@ -46,6 +47,7 @@
#include "mathops.h"
#include "tiff.h"
#include "tiff_data.h"
+#include "mjpegdec.h"
#include "thread.h"
#include "get_bits.h"
@@ -54,6 +56,10 @@ typedef struct TiffContext {
AVCodecContext *avctx;
GetByteContext gb;
+/* JPEG decoding for DNG */
+AVCodecContext *avctx_mjpeg; // wrapper context for MJPEG
+AVFrame *jpgframe; // decoded JPEG tile
+
int get_subimage;
uint16_t get_page;
int get_thumbnail;
@@ -76,7 +82,9 @@ typedef struct TiffContext {
int is_bayer;
uint8_t pattern[4];
+unsigned black_level;
unsigned white_level;
+const uint16_t *dng_lut; // Pointer to DNG linearization table
uint32_t sub_ifd;
uint16_t cur_page;
@@ -86,6 +94,14 @@ typedef struct TiffContext {
int stripsizesoff, stripsize, stripoff, strippos;
LZWState *lzw;
+/* Tile support */
+int is_tiled;
+int tile_byte_counts_offset, tile_offsets_offset;
+int tile_width, tile_length;
+int tile_count;
+
+int is_jpeg;
+
uint8_t *deinvert_buf;
int deinvert_buf_size;
uint8_t *yuv_line;
@@ -257,6 +273,9 @@ static int add_metadata(int count, int type,
};
}
+static void av_always_inline dng_blit(TiffContext *s, uint8_t *dst, int
dst_stride,
+ const uint8_t *src, int src_stride, int
width, int height, int is_u16);
+
static void av_always_inline horizontal_fill(TiffContext *s,
unsigned int bpp, uint8_t* dst,
int usePtr, const uint8_t *src,
@@ -712,6 +731,204 @@ static int tiff_unpack_strip(TiffContext *s, AVFrame *p,
uint8_t *dst, int strid
return 0;
}
+/**
+ * Map stored raw sensor values into linear reference values.
+ * See: DNG Specification - Chapter 5
+ */
+static uint16_t av_always_inline dng_raw_to_linear16(uint16_t value,
+const uint16_t *lut,
+uint16_t black_level,
+float scale_factor) {
+// Lookup table lookup
+if (lut)
+value = lut[value];
+
+// Black level subtraction
+value = av_clip_uint16_c((unsigned)value - black_level);
+
+// Color scaling
+value = av_clip_uint16_c((unsigned)(((float)value * scale_factor) *
0x));
+
+return value;
+}
+
+static uint16_t av_always_inline dng_raw_to_linear8(uint16_t value,
+const uint16_t *lut,
+uint16_t black_level,
+float scale_factor) {
+return dng_raw_to_linear16(value, lut, black_level, scale_factor) >> 8;
+}
+
+static void dng_blit(TiffContext *s,
