| Message ID | 20210808172941.18238-10-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/10] ffmpeg: reset the dict iterator before use | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On Sun, Aug 08, 2021 at 07:29:41PM +0200, Anton Khirnov wrote: > --- > libavfilter/vf_scale.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c > index b62fb37d4b..14e202bf77 100644 > --- a/libavfilter/vf_scale.c > +++ b/libavfilter/vf_scale.c > @@ -542,6 +542,7 @@ static int config_props(AVFilterLink *outlink) > av_opt_set_int(*s, "sws_flags", scale->flags, 0); > av_opt_set_int(*s, "param0", scale->param[0], 0); > av_opt_set_int(*s, "param1", scale->param[1], 0); > + av_opt_set_int(*s, "threads", ff_filter_get_nb_threads(ctx), 0); > if (scale->in_range != AVCOL_RANGE_UNSPECIFIED) > av_opt_set_int(*s, "src_range", > scale->in_range == AVCOL_RANGE_JPEG, 0); > -- > 2.30.2 breaks: ./ffmpeg -i ~/tickets/1012/IV50_random_points.avi -threads 5 -y file1012.avi it contains horizontal bright green lines [...]
Quoting Michael Niedermayer (2021-08-09 22:30:06) > On Sun, Aug 08, 2021 at 07:29:41PM +0200, Anton Khirnov wrote: > > --- > > libavfilter/vf_scale.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c > > index b62fb37d4b..14e202bf77 100644 > > --- a/libavfilter/vf_scale.c > > +++ b/libavfilter/vf_scale.c > > @@ -542,6 +542,7 @@ static int config_props(AVFilterLink *outlink) > > av_opt_set_int(*s, "sws_flags", scale->flags, 0); > > av_opt_set_int(*s, "param0", scale->param[0], 0); > > av_opt_set_int(*s, "param1", scale->param[1], 0); > > + av_opt_set_int(*s, "threads", ff_filter_get_nb_threads(ctx), 0); > > if (scale->in_range != AVCOL_RANGE_UNSPECIFIED) > > av_opt_set_int(*s, "src_range", > > scale->in_range == AVCOL_RANGE_JPEG, 0); > > -- > > 2.30.2 > > breaks: > ./ffmpeg -i ~/tickets/1012/IV50_random_points.avi -threads 5 -y file1012.avi > > it contains horizontal bright green lines Should be fixed with the updated patches I sent just now. That said, I think the special 410->420 scaler should be dropped - its output fundamentally depends on how the slices are submitted. Given that 410 is a pretty obscure format, it's unlikely anyone needs a super-fast non-exact conversion.
On Sun, Aug 29, 2021 at 06:48:36PM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2021-08-09 22:30:06) > > On Sun, Aug 08, 2021 at 07:29:41PM +0200, Anton Khirnov wrote: > > > --- > > > libavfilter/vf_scale.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c > > > index b62fb37d4b..14e202bf77 100644 > > > --- a/libavfilter/vf_scale.c > > > +++ b/libavfilter/vf_scale.c > > > @@ -542,6 +542,7 @@ static int config_props(AVFilterLink *outlink) > > > av_opt_set_int(*s, "sws_flags", scale->flags, 0); > > > av_opt_set_int(*s, "param0", scale->param[0], 0); > > > av_opt_set_int(*s, "param1", scale->param[1], 0); > > > + av_opt_set_int(*s, "threads", ff_filter_get_nb_threads(ctx), 0); > > > if (scale->in_range != AVCOL_RANGE_UNSPECIFIED) > > > av_opt_set_int(*s, "src_range", > > > scale->in_range == AVCOL_RANGE_JPEG, 0); > > > -- > > > 2.30.2 > > > > breaks: > > ./ffmpeg -i ~/tickets/1012/IV50_random_points.avi -threads 5 -y file1012.avi > > > > it contains horizontal bright green lines > > Should be fixed with the updated patches I sent just now. > > That said, I think the special 410->420 scaler should be dropped - its > output fundamentally depends on how the slices are submitted. Given that hmm, iam not sure i understand the issue you describe. Is this because the 410->420 scaler interpolates the chroma and to do so it has to handle the image borders differently? That is done by the standard code path too and would be the expected "high quality" thing to do. It is more noticable with 410 because chroma is subsampled so extreemly so artifacts are more vissible but for best quality we should interpolate all the chroma subsample convertions by default. But maybe there is some other issue in this converter which i forgot ... > 410 is a pretty obscure format, it's unlikely anyone needs a super-fast > non-exact conversion. [...]
Quoting Michael Niedermayer (2021-08-29 22:22:04) > On Sun, Aug 29, 2021 at 06:48:36PM +0200, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2021-08-09 22:30:06) > > > On Sun, Aug 08, 2021 at 07:29:41PM +0200, Anton Khirnov wrote: > > > > --- > > > > libavfilter/vf_scale.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c > > > > index b62fb37d4b..14e202bf77 100644 > > > > --- a/libavfilter/vf_scale.c > > > > +++ b/libavfilter/vf_scale.c > > > > @@ -542,6 +542,7 @@ static int config_props(AVFilterLink *outlink) > > > > av_opt_set_int(*s, "sws_flags", scale->flags, 0); > > > > av_opt_set_int(*s, "param0", scale->param[0], 0); > > > > av_opt_set_int(*s, "param1", scale->param[1], 0); > > > > + av_opt_set_int(*s, "threads", ff_filter_get_nb_threads(ctx), 0); > > > > if (scale->in_range != AVCOL_RANGE_UNSPECIFIED) > > > > av_opt_set_int(*s, "src_range", > > > > scale->in_range == AVCOL_RANGE_JPEG, 0); > > > > -- > > > > 2.30.2 > > > > > > breaks: > > > ./ffmpeg -i ~/tickets/1012/IV50_random_points.avi -threads 5 -y file1012.avi > > > > > > it contains horizontal bright green lines > > > > Should be fixed with the updated patches I sent just now. > > > > > That said, I think the special 410->420 scaler should be dropped - its > > output fundamentally depends on how the slices are submitted. Given that > > hmm, iam not sure i understand the issue you describe. > Is this because the 410->420 scaler interpolates the chroma and to do so > it has to handle the image borders differently? It handles _slice_ borders differently, not image borders. So the result depends on how the image is partitioned into slices. I am not familiar with the generic scaler code, but it seems independent of this partitioning, otherwise the threaded scaling tests would fail.
On Mon, Aug 30, 2021 at 10:34:18AM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2021-08-29 22:22:04) > > On Sun, Aug 29, 2021 at 06:48:36PM +0200, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2021-08-09 22:30:06) > > > > On Sun, Aug 08, 2021 at 07:29:41PM +0200, Anton Khirnov wrote: > > > > > --- > > > > > libavfilter/vf_scale.c | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c > > > > > index b62fb37d4b..14e202bf77 100644 > > > > > --- a/libavfilter/vf_scale.c > > > > > +++ b/libavfilter/vf_scale.c > > > > > @@ -542,6 +542,7 @@ static int config_props(AVFilterLink *outlink) > > > > > av_opt_set_int(*s, "sws_flags", scale->flags, 0); > > > > > av_opt_set_int(*s, "param0", scale->param[0], 0); > > > > > av_opt_set_int(*s, "param1", scale->param[1], 0); > > > > > + av_opt_set_int(*s, "threads", ff_filter_get_nb_threads(ctx), 0); > > > > > if (scale->in_range != AVCOL_RANGE_UNSPECIFIED) > > > > > av_opt_set_int(*s, "src_range", > > > > > scale->in_range == AVCOL_RANGE_JPEG, 0); > > > > > -- > > > > > 2.30.2 > > > > > > > > breaks: > > > > ./ffmpeg -i ~/tickets/1012/IV50_random_points.avi -threads 5 -y file1012.avi > > > > > > > > it contains horizontal bright green lines > > > > > > Should be fixed with the updated patches I sent just now. > > > > > > > > That said, I think the special 410->420 scaler should be dropped - its > > > output fundamentally depends on how the slices are submitted. Given that > > > > hmm, iam not sure i understand the issue you describe. > > Is this because the 410->420 scaler interpolates the chroma and to do so > > it has to handle the image borders differently? > > It handles _slice_ borders differently, not image borders. So the result > depends on how the image is partitioned into slices. > > I am not familiar with the generic scaler code, but it seems independent > of this partitioning, otherwise the threaded scaling tests would fail. the generic scaler simply stores the data originating from the previous slice (generally after the horizontal scaler) the 410->420 one probably should * store the one chroma line too somewhere * initialize it to the first image line * simplify all the 410->420 code so it always uses a pointer to the previous line either from the buffer or if available straight from the input image It seems not worth for just 410->420, and i agree but the same could be used for 420->444 and others which would have the same problem if one wanted to do higher quality chroma interpolation in the unscaled special converters thx [...]
On Mon, Aug 30, 2021 at 11:38:53AM +0200, Michael Niedermayer wrote: > On Mon, Aug 30, 2021 at 10:34:18AM +0200, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2021-08-29 22:22:04) > > > On Sun, Aug 29, 2021 at 06:48:36PM +0200, Anton Khirnov wrote: > > > > Quoting Michael Niedermayer (2021-08-09 22:30:06) > > > > > On Sun, Aug 08, 2021 at 07:29:41PM +0200, Anton Khirnov wrote: > > > > > > --- > > > > > > libavfilter/vf_scale.c | 1 + > > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > > > diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c > > > > > > index b62fb37d4b..14e202bf77 100644 > > > > > > --- a/libavfilter/vf_scale.c > > > > > > +++ b/libavfilter/vf_scale.c > > > > > > @@ -542,6 +542,7 @@ static int config_props(AVFilterLink *outlink) > > > > > > av_opt_set_int(*s, "sws_flags", scale->flags, 0); > > > > > > av_opt_set_int(*s, "param0", scale->param[0], 0); > > > > > > av_opt_set_int(*s, "param1", scale->param[1], 0); > > > > > > + av_opt_set_int(*s, "threads", ff_filter_get_nb_threads(ctx), 0); > > > > > > if (scale->in_range != AVCOL_RANGE_UNSPECIFIED) > > > > > > av_opt_set_int(*s, "src_range", > > > > > > scale->in_range == AVCOL_RANGE_JPEG, 0); > > > > > > -- > > > > > > 2.30.2 > > > > > > > > > > breaks: > > > > > ./ffmpeg -i ~/tickets/1012/IV50_random_points.avi -threads 5 -y file1012.avi > > > > > > > > > > it contains horizontal bright green lines > > > > > > > > Should be fixed with the updated patches I sent just now. > > > > > > > > > > > That said, I think the special 410->420 scaler should be dropped - its > > > > output fundamentally depends on how the slices are submitted. Given that > > > > > > hmm, iam not sure i understand the issue you describe. > > > Is this because the 410->420 scaler interpolates the chroma and to do so > > > it has to handle the image borders differently? > > > > It handles _slice_ borders differently, not image borders. So the result > > depends on how the image is partitioned into slices. > > > > > I am not familiar with the generic scaler code, but it seems independent > > of this partitioning, otherwise the threaded scaling tests would fail. > > the generic scaler simply stores the data originating from the previous slice > (generally after the horizontal scaler) > > the 410->420 one probably should > * store the one chroma line too somewhere > * initialize it to the first image line > * simplify all the 410->420 code so it always uses a pointer to the previous > line either from the buffer or if available straight from the input image > > It seems not worth for just 410->420, and i agree but the same could > be used for 420->444 and others which would have the same problem if one > wanted to do higher quality chroma interpolation in the unscaled special > converters Just to clarify, this is meant as a path forward for the bug with chroma interpolation in the special converters which do or might want to use chroma interpolation. Its not a review comment to the patchset(s) thx [...]
Quoting Michael Niedermayer (2021-08-30 14:34:30) > On Mon, Aug 30, 2021 at 11:38:53AM +0200, Michael Niedermayer wrote: > > > I am not familiar with the generic scaler code, but it seems independent > > > of this partitioning, otherwise the threaded scaling tests would fail. > > > > the generic scaler simply stores the data originating from the previous slice > > (generally after the horizontal scaler) > > > > the 410->420 one probably should > > * store the one chroma line too somewhere > > * initialize it to the first image line > > * simplify all the 410->420 code so it always uses a pointer to the previous > > line either from the buffer or if available straight from the input image > > > > It seems not worth for just 410->420, and i agree but the same could > > be used for 420->444 and others which would have the same problem if one > > wanted to do higher quality chroma interpolation in the unscaled special > > converters > > Just to clarify, this is meant as a path forward for the bug with chroma > interpolation in the special converters which do or might want to use > chroma interpolation. > Its not a review comment to the patchset(s) Right, that's how I understood this. Are you still looking at the patchset or shall I go ahead with pushing it to master?
On Wed, Sep 01, 2021 at 10:26:25AM +0200, Anton Khirnov wrote: > Quoting Michael Niedermayer (2021-08-30 14:34:30) > > On Mon, Aug 30, 2021 at 11:38:53AM +0200, Michael Niedermayer wrote: > > > > I am not familiar with the generic scaler code, but it seems independent > > > > of this partitioning, otherwise the threaded scaling tests would fail. > > > > > > the generic scaler simply stores the data originating from the previous slice > > > (generally after the horizontal scaler) > > > > > > the 410->420 one probably should > > > * store the one chroma line too somewhere > > > * initialize it to the first image line > > > * simplify all the 410->420 code so it always uses a pointer to the previous > > > line either from the buffer or if available straight from the input image > > > > > > It seems not worth for just 410->420, and i agree but the same could > > > be used for 420->444 and others which would have the same problem if one > > > wanted to do higher quality chroma interpolation in the unscaled special > > > converters > > > > Just to clarify, this is meant as a path forward for the bug with chroma > > interpolation in the special converters which do or might want to use > > chroma interpolation. > > Its not a review comment to the patchset(s) > > Right, that's how I understood this. > > Are you still looking at the patchset or shall I go ahead with pushing > it to master? ive posted some comments and tested it, i didnt see any issues beyond what was in my 2 replies thx [...]
diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c index b62fb37d4b..14e202bf77 100644 --- a/libavfilter/vf_scale.c +++ b/libavfilter/vf_scale.c @@ -542,6 +542,7 @@ static int config_props(AVFilterLink *outlink) av_opt_set_int(*s, "sws_flags", scale->flags, 0); av_opt_set_int(*s, "param0", scale->param[0], 0); av_opt_set_int(*s, "param1", scale->param[1], 0); + av_opt_set_int(*s, "threads", ff_filter_get_nb_threads(ctx), 0); if (scale->in_range != AVCOL_RANGE_UNSPECIFIED) av_opt_set_int(*s, "src_range", scale->in_range == AVCOL_RANGE_JPEG, 0);