| Message ID | 589892C6.9080608@m1stereo.tv |
|---|---|
| State | Accepted |
| Commit | 8efb7f5a26e7300e09ba01239328d1498238c532 |
| Headers | show |
On Mon, Feb 06, 2017 at 05:14:14PM +0200, Maksym Veremeyenko wrote: > Hi, > > Attached patch fixes chroma positioning during scaling interlaced 4:2:0. > > On a first iteration default context value been overwritten by new > value and not been update on next iterations for fields. This mean > that vertical chroma position remain 128 for field#0 and field#1 > instead of *64* and *192*. > > Attached patch use local variable for storing this intermediate > value of chroma vertical position not modifying default context > value. > > -- > Maksym Veremeyenko > > vf_scale.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > 6a3a3966939dd87c45a8e124b0364da30b02728b 0001-Fix-chroma-positioning-for-4-2-0-pixel-format.patch > From 912ecf538b6b2f7a8df4afdfed2d34052162335c Mon Sep 17 00:00:00 2001 > From: Maksym Veremeyenko <verem@m1.tv> > Date: Mon, 6 Feb 2017 17:03:17 +0200 > Subject: [PATCH] Fix chroma positioning for 4:2:0 pixel format applied thx [...]
Michael Niedermayer wrote: > On Mon, Feb 06, 2017 at 05:14:14PM +0200, Maksym Veremeyenko wrote: >> Hi, >> >> Attached patch fixes chroma positioning during scaling interlaced 4:2:0. >> >> On a first iteration default context value been overwritten by new >> value and not been update on next iterations for fields. This mean >> that vertical chroma position remain 128 for field#0 and field#1 >> instead of *64* and *192*. >> >> Attached patch use local variable for storing this intermediate >> value of chroma vertical position not modifying default context >> value. >> >> -- >> Maksym Veremeyenko >> > >> vf_scale.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> 6a3a3966939dd87c45a8e124b0364da30b02728b 0001-Fix-chroma-positioning-for-4-2-0-pixel-format.patch >> From 912ecf538b6b2f7a8df4afdfed2d34052162335c Mon Sep 17 00:00:00 2001 >> From: Maksym Veremeyenko <verem@m1.tv> >> Date: Mon, 6 Feb 2017 17:03:17 +0200 >> Subject: [PATCH] Fix chroma positioning for 4:2:0 pixel format > > applied > > thx Nice, testing wise it raises a question to how swscale should work by default with interl=1 and 420 to rgb (maybe other cases as well). In summary this patch will appear to fail with a test like. ffmpeg -i 420-interlaced.ts -ss 11.2 -vf scale=interl=1 -vframes 1 out.png Will look just as broken as before the patch. The addition of - -sws_flags +accurate_rnd fixes it, adding +full_chroma_int looks better still.
Andy Furniss wrote: > Michael Niedermayer wrote: >> On Mon, Feb 06, 2017 at 05:14:14PM +0200, Maksym Veremeyenko wrote: >>> Hi, >>> >>> Attached patch fixes chroma positioning during scaling interlaced 4:2:0. >>> >>> On a first iteration default context value been overwritten by new >>> value and not been update on next iterations for fields. This mean >>> that vertical chroma position remain 128 for field#0 and field#1 >>> instead of *64* and *192*. >>> >>> Attached patch use local variable for storing this intermediate >>> value of chroma vertical position not modifying default context >>> value. >>> >>> -- >>> Maksym Veremeyenko >>> >> >>> vf_scale.c | 9 +++++---- >>> 1 file changed, 5 insertions(+), 4 deletions(-) >>> 6a3a3966939dd87c45a8e124b0364da30b02728b >>> 0001-Fix-chroma-positioning-for-4-2-0-pixel-format.patch >>> From 912ecf538b6b2f7a8df4afdfed2d34052162335c Mon Sep 17 00:00:00 2001 >>> From: Maksym Veremeyenko <verem@m1.tv> >>> Date: Mon, 6 Feb 2017 17:03:17 +0200 >>> Subject: [PATCH] Fix chroma positioning for 4:2:0 pixel format >> >> applied >> >> thx > > Nice, testing wise it raises a question to how swscale should work by > default with interl=1 and 420 to rgb (maybe other cases as well). > > In summary this patch will appear to fail with a test like. > > ffmpeg -i 420-interlaced.ts -ss 11.2 -vf scale=interl=1 -vframes 1 out.png > > Will look just as broken as before the patch. The addition of - > > -sws_flags +accurate_rnd > > fixes it, adding +full_chroma_int looks better still. Another possible issue which I haven't fully investigated = This doesn't obey setfield, which makes me think that maybe it's only working by luck on tff input. A quick test with bff does seem wrong, but my bff sample may be to blame and I don't have any "real" bff with the "right content" to test.
Andy Furniss wrote: > Another possible issue which I haven't fully investigated = > > This doesn't obey setfield, which makes me think that maybe it's > only working by luck on tff input. Ugh, ignore that bit as I don't think it should matter as long as the filter always sees weaved frames. > A quick test with bff does seem wrong, but my bff sample may be to > blame and I don't have any "real" bff with the "right content" to test. I guess the sample I looked at is flawed, will have to find more.
diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c index 22bee96..a7dfd3d 100644 --- a/libavfilter/vf_scale.c +++ b/libavfilter/vf_scale.c @@ -374,6 +374,7 @@ static int config_props(AVFilterLink *outlink) int i; for (i = 0; i < 3; i++) { + int in_v_chr_pos = scale->in_v_chr_pos, out_v_chr_pos = scale->out_v_chr_pos; struct SwsContext **s = swscs[i]; *s = sws_alloc_context(); if (!*s) @@ -406,17 +407,17 @@ static int config_props(AVFilterLink *outlink) * MPEG-2 chroma positions are used by convention * XXX: support other 4:2:0 pixel formats */ if (inlink0->format == AV_PIX_FMT_YUV420P && scale->in_v_chr_pos == -513) { - scale->in_v_chr_pos = (i == 0) ? 128 : (i == 1) ? 64 : 192; + in_v_chr_pos = (i == 0) ? 128 : (i == 1) ? 64 : 192; } if (outlink->format == AV_PIX_FMT_YUV420P && scale->out_v_chr_pos == -513) { - scale->out_v_chr_pos = (i == 0) ? 128 : (i == 1) ? 64 : 192; + out_v_chr_pos = (i == 0) ? 128 : (i == 1) ? 64 : 192; } av_opt_set_int(*s, "src_h_chr_pos", scale->in_h_chr_pos, 0); - av_opt_set_int(*s, "src_v_chr_pos", scale->in_v_chr_pos, 0); + av_opt_set_int(*s, "src_v_chr_pos", in_v_chr_pos, 0); av_opt_set_int(*s, "dst_h_chr_pos", scale->out_h_chr_pos, 0); - av_opt_set_int(*s, "dst_v_chr_pos", scale->out_v_chr_pos, 0); + av_opt_set_int(*s, "dst_v_chr_pos", out_v_chr_pos, 0); if ((ret = sws_init_context(*s, NULL, NULL)) < 0) return ret;