| Message ID | 20220105151506.74794-1-vittorio.giovara@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] vf_tonemap: Fix order of planes | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
On Wed, Jan 5, 2022 at 4:15 PM Vittorio Giovara <vittorio.giovara@gmail.com> wrote: > This resulted in a dimmed tonemapping due to bad resulting luma > calculation. > > Found by: Derek Buitenhuis > > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> > --- > libavfilter/vf_tonemap.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/libavfilter/vf_tonemap.c b/libavfilter/vf_tonemap.c > index 363df8034b..644c9fd9e3 100644 > --- a/libavfilter/vf_tonemap.c > +++ b/libavfilter/vf_tonemap.c > @@ -120,17 +120,17 @@ static void tonemap(TonemapContext *s, AVFrame *out, > const AVFrame *in, > const AVPixFmtDescriptor *desc, int x, int y, double > peak) > { > const float *r_in = (const float *)(in->data[0] + x * > desc->comp[0].step + y * in->linesize[0]); > - const float *b_in = (const float *)(in->data[1] + x * > desc->comp[1].step + y * in->linesize[1]); > - const float *g_in = (const float *)(in->data[2] + x * > desc->comp[2].step + y * in->linesize[2]); > + const float *g_in = (const float *)(in->data[1] + x * > desc->comp[1].step + y * in->linesize[1]); > + const float *b_in = (const float *)(in->data[2] + x * > desc->comp[2].step + y * in->linesize[2]); > float *r_out = (float *)(out->data[0] + x * desc->comp[0].step + y * > out->linesize[0]); > - float *b_out = (float *)(out->data[1] + x * desc->comp[1].step + y * > out->linesize[1]); > - float *g_out = (float *)(out->data[2] + x * desc->comp[2].step + y * > out->linesize[2]); > + float *g_out = (float *)(out->data[1] + x * desc->comp[1].step + y * > out->linesize[1]); > + float *b_out = (float *)(out->data[2] + x * desc->comp[2].step + y * > out->linesize[2]); > float sig, sig_orig; > > /* load values */ > *r_out = *r_in; > - *b_out = *b_in; > *g_out = *g_in; > + *b_out = *b_in; > > /* desaturate to prevent unnatural colors */ > if (s->desat > 0) { > -- > 2.34.1 > > Pushing soon unless objections. Possibly backporting it to 5.0 too.
Quoting Vittorio Giovara (2022-01-11 18:48:27)
> Pushing soon unless objections. Possibly backporting it to 5.0 too.
Could you add a test?
Quoting Vittorio Giovara (2022-01-05 16:15:06) > This resulted in a dimmed tonemapping due to bad resulting luma > calculation. > > Found by: Derek Buitenhuis > > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> > --- > libavfilter/vf_tonemap.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/libavfilter/vf_tonemap.c b/libavfilter/vf_tonemap.c > index 363df8034b..644c9fd9e3 100644 > --- a/libavfilter/vf_tonemap.c > +++ b/libavfilter/vf_tonemap.c > @@ -120,17 +120,17 @@ static void tonemap(TonemapContext *s, AVFrame *out, const AVFrame *in, > const AVPixFmtDescriptor *desc, int x, int y, double peak) > { > const float *r_in = (const float *)(in->data[0] + x * desc->comp[0].step + y * in->linesize[0]); > - const float *b_in = (const float *)(in->data[1] + x * desc->comp[1].step + y * in->linesize[1]); > - const float *g_in = (const float *)(in->data[2] + x * desc->comp[2].step + y * in->linesize[2]); > + const float *g_in = (const float *)(in->data[1] + x * desc->comp[1].step + y * in->linesize[1]); > + const float *b_in = (const float *)(in->data[2] + x * desc->comp[2].step + y * in->linesize[2]); I'm confused by this, the filter seems to use GBR pixel format.
On Wed, Jan 12, 2022 at 10:47 AM Anton Khirnov <anton@khirnov.net> wrote: > Quoting Vittorio Giovara (2022-01-11 18:48:27) > > Pushing soon unless objections. Possibly backporting it to 5.0 too. > > Could you add a test? > The filter works in floating point space so it's a bit complex to do so. Maybe it could be optimized in fixed point and we could add a test then
On Wed, Jan 12, 2022 at 10:51 AM Anton Khirnov <anton@khirnov.net> wrote: > Quoting Vittorio Giovara (2022-01-05 16:15:06) > > This resulted in a dimmed tonemapping due to bad resulting luma > > calculation. > > > > Found by: Derek Buitenhuis > > > > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> > > --- > > libavfilter/vf_tonemap.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/libavfilter/vf_tonemap.c b/libavfilter/vf_tonemap.c > > index 363df8034b..644c9fd9e3 100644 > > --- a/libavfilter/vf_tonemap.c > > +++ b/libavfilter/vf_tonemap.c > > @@ -120,17 +120,17 @@ static void tonemap(TonemapContext *s, AVFrame > *out, const AVFrame *in, > > const AVPixFmtDescriptor *desc, int x, int y, > double peak) > > { > > const float *r_in = (const float *)(in->data[0] + x * > desc->comp[0].step + y * in->linesize[0]); > > - const float *b_in = (const float *)(in->data[1] + x * > desc->comp[1].step + y * in->linesize[1]); > > - const float *g_in = (const float *)(in->data[2] + x * > desc->comp[2].step + y * in->linesize[2]); > > + const float *g_in = (const float *)(in->data[1] + x * > desc->comp[1].step + y * in->linesize[1]); > > + const float *b_in = (const float *)(in->data[2] + x * > desc->comp[2].step + y * in->linesize[2]); > > I'm confused by this, the filter seems to use GBR pixel format. > hmmm right, so the first and last elements are swapped... i'm possibly implying the order in a wrong way, let me send a more generic version of this then
Quoting Vittorio Giovara (2022-01-12 14:43:39) > On Wed, Jan 12, 2022 at 10:47 AM Anton Khirnov <anton@khirnov.net> wrote: > > > Quoting Vittorio Giovara (2022-01-11 18:48:27) > > > Pushing soon unless objections. Possibly backporting it to 5.0 too. > > > > Could you add a test? > > > > The filter works in floating point space so it's a bit complex to do so. > Maybe it could be optimized in fixed point and we could add a test then Is it? I see the owdenoise filter test using oneoff comparison for floats, that might be easy to adapt for tonemap.
diff --git a/libavfilter/vf_tonemap.c b/libavfilter/vf_tonemap.c index 363df8034b..644c9fd9e3 100644 --- a/libavfilter/vf_tonemap.c +++ b/libavfilter/vf_tonemap.c @@ -120,17 +120,17 @@ static void tonemap(TonemapContext *s, AVFrame *out, const AVFrame *in, const AVPixFmtDescriptor *desc, int x, int y, double peak) { const float *r_in = (const float *)(in->data[0] + x * desc->comp[0].step + y * in->linesize[0]); - const float *b_in = (const float *)(in->data[1] + x * desc->comp[1].step + y * in->linesize[1]); - const float *g_in = (const float *)(in->data[2] + x * desc->comp[2].step + y * in->linesize[2]); + const float *g_in = (const float *)(in->data[1] + x * desc->comp[1].step + y * in->linesize[1]); + const float *b_in = (const float *)(in->data[2] + x * desc->comp[2].step + y * in->linesize[2]); float *r_out = (float *)(out->data[0] + x * desc->comp[0].step + y * out->linesize[0]); - float *b_out = (float *)(out->data[1] + x * desc->comp[1].step + y * out->linesize[1]); - float *g_out = (float *)(out->data[2] + x * desc->comp[2].step + y * out->linesize[2]); + float *g_out = (float *)(out->data[1] + x * desc->comp[1].step + y * out->linesize[1]); + float *b_out = (float *)(out->data[2] + x * desc->comp[2].step + y * out->linesize[2]); float sig, sig_orig; /* load values */ *r_out = *r_in; - *b_out = *b_in; *g_out = *g_in; + *b_out = *b_in; /* desaturate to prevent unnatural colors */ if (s->desat > 0) {
This resulted in a dimmed tonemapping due to bad resulting luma calculation. Found by: Derek Buitenhuis Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com> --- libavfilter/vf_tonemap.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)