[FFmpeg-devel] [RFC][PATCH] Use correct chroma position in YUV420P interlaced conversions

[Kieran Kunhya]

I think this is right but would be useful to get it checked. Visually the output looks ok

---
libavfilter/vf_scale.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
index 64b88c2..83a0666 100644
--- a/libavfilter/vf_scale.c
+++ b/libavfilter/vf_scale.c
@@ -373,6 +373,13 @@ static int config_props(AVFilterLink *outlink)
av_opt_set_int(*s, "dst_format", outfmt, 0);
av_opt_set_int(*s, "sws_flags", scale->flags, 0);

+ /* Override interlaced YUV420P settings to have the correct (MPEG-2) chroma positions
+ * MPEG-2 chroma positions are used by convention
+ * Set the context up for tff */
+ if (i && scale->interlaced && inlink->format == AV_PIX_FMT_YUV420P){
+ scale->in_v_chr_pos = (i == 1) ? 64 : -64;
+ }
+
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, "dst_h_chr_pos", scale->out_h_chr_pos, 0);
@@ -418,7 +425,12 @@ static int scale_slice(AVFilterLink *link, AVFrame *out_buf, AVFrame *cur_pic, s
int vsub= ((i+1)&2) ? scale->vsub : 0;
in_stride[i] = cur_pic->linesize[i] * mul;
out_stride[i] = out_buf->linesize[i] * mul;
- in[i] = cur_pic->data[i] + ((y>>vsub)+field) * cur_pic->linesize[i];
+
+ /* Chroma planes are shared in interlaced 4:2:0 */
+ if (i && cur_pic->format == AV_PIX_FMT_YUV420P && scale->interlaced)
+ in[i] = cur_pic->data[i];
+ else
+ in[i] = cur_pic->data[i] + ((y>>vsub)+field) * cur_pic->linesize[i];
out[i] = out_buf->data[i] + field * out_buf->linesize[i];
}
if(scale->input_is_pal)
@@ -520,8 +532,8 @@ static int filter_frame(AVFilterLink *link, AVFrame *in)
INT_MAX);

if(scale->interlaced>0 || (scale->interlaced<0 && in->interlaced_frame)){
- scale_slice(link, out, in, scale->isws[0], 0, (link->h+1)/2, 2, 0);
- scale_slice(link, out, in, scale->isws[1], 0, link->h /2, 2, 1);
+ scale_slice(link, out, in, scale->isws[!in->top_field_first], 0, (link->h+1)/2, 2, 0);
+ scale_slice(link, out, in, scale->isws[in->top_field_first], 0, link->h /2, 2, 1);
}else{
scale_slice(link, out, in, scale->sws, 0, link->h, 1, 0);
}

[Carl Eugen Hoyos]

Post by Kieran Kunhya
I think this is right but would be useful to
get it checked. Visually the output looks ok

Is this related to ticket #4168?

If yes, please add a comment.

Thank you, Carl Eugen

[Kieran Kunhya]

Fixed wrong chroma line use
---
libavfilter/vf_scale.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
index 64b88c2..9189103 100644
--- a/libavfilter/vf_scale.c
+++ b/libavfilter/vf_scale.c
@@ -373,6 +373,13 @@ static int config_props(AVFilterLink *outlink)
av_opt_set_int(*s, "dst_format", outfmt, 0);
av_opt_set_int(*s, "sws_flags", scale->flags, 0);

+ /* Override interlaced YUV420P settings to have the correct (MPEG-2) chroma positions
+ * MPEG-2 chroma positions are used by convention
+ * Set the context up for tff */
+ if (i && scale->interlaced && inlink->format == AV_PIX_FMT_YUV420P){
+ scale->in_v_chr_pos = (i == 1) ? 64 : -64;
+ }
+
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, "dst_h_chr_pos", scale->out_h_chr_pos, 0);
@@ -520,8 +527,8 @@ static int filter_frame(AVFilterLink *link, AVFrame *in)
INT_MAX);

if(scale->interlaced>0 || (scale->interlaced<0 && in->interlaced_frame)){
- scale_slice(link, out, in, scale->isws[0], 0, (link->h+1)/2, 2, 0);
- scale_slice(link, out, in, scale->isws[1], 0, link->h /2, 2, 1);
+ scale_slice(link, out, in, scale->isws[!in->top_field_first], 0, (link->h+1)/2, 2, 0);
+ scale_slice(link, out, in, scale->isws[in->top_field_first], 0, link->h /2, 2, 1);
}else{
scale_slice(link, out, in, scale->sws, 0, link->h, 1, 0);
}

[Michael Niedermayer]

Post by Kieran Kunhya
Fixed wrong chroma line use
---
libavfilter/vf_scale.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/libavfilter/vf_scale.c b/libavfilter/vf_scale.c
index 64b88c2..9189103 100644
--- a/libavfilter/vf_scale.c
+++ b/libavfilter/vf_scale.c
@@ -373,6 +373,13 @@ static int config_props(AVFilterLink *outlink)
av_opt_set_int(*s, "dst_format", outfmt, 0);
av_opt_set_int(*s, "sws_flags", scale->flags, 0);
+ /* Override interlaced YUV420P settings to have the correct (MPEG-2) chroma positions
+ * MPEG-2 chroma positions are used by convention
+ * Set the context up for tff */
+ if (i && scale->interlaced && inlink->format == AV_PIX_FMT_YUV420P){
+ scale->in_v_chr_pos = (i == 1) ? 64 : -64;
+ }

this should check if in_v_chr_pos is the default, and maybe either
not change it if its not or print a warning to inform the user that
the value she specified is not being used
maybe av_opt_is_set_to_default_by_name() could be used
the default also could be changed to detect if the user explicitly
specified the "default" value but maybe thats overkill feel free to
ignore

Post by Kieran Kunhya
+
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, "dst_h_chr_pos", scale->out_h_chr_pos, 0);
@@ -520,8 +527,8 @@ static int filter_frame(AVFilterLink *link, AVFrame *in)
INT_MAX);
if(scale->interlaced>0 || (scale->interlaced<0 && in->interlaced_frame)){
- scale_slice(link, out, in, scale->isws[0], 0, (link->h+1)/2, 2, 0);
- scale_slice(link, out, in, scale->isws[1], 0, link->h /2, 2, 1);
+ scale_slice(link, out, in, scale->isws[!in->top_field_first], 0, (link->h+1)/2, 2, 0);
+ scale_slice(link, out, in, scale->isws[in->top_field_first], 0, link->h /2, 2, 1);
}else{

are you sure this is correct ?
maybe i misunderstand but it looks a bit odd
i think the upper field(s) should be treated the same no matter if
its temporally first or second

[...]

[Kieran Kunhya]

Post by Michael Niedermayer

Post by Kieran Kunhya
+
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, "dst_h_chr_pos", scale->out_h_chr_pos, 0);
@@ -520,8 +527,8 @@ static int filter_frame(AVFilterLink *link, AVFrame *in)
INT_MAX);
if(scale->interlaced>0 || (scale->interlaced<0 && in->interlaced_frame)){
- scale_slice(link, out, in, scale->isws[0], 0, (link->h+1)/2, 2, 0);
- scale_slice(link, out, in, scale->isws[1], 0, link->h /2, 2, 1);
+ scale_slice(link, out, in, scale->isws[!in->top_field_first], 0, (link->h+1)/2, 2, 0);
+ scale_slice(link, out, in, scale->isws[in->top_field_first], 0, link->h /2, 2, 1);
}else{

are you sure this is correct ?
maybe i misunderstand but it looks a bit odd
i think the upper field(s) should be treated the same no matter if
its temporally first or second

If the lower field is the first line, then the lower field has a
different chroma position.

[Kieran Kunhya]

This post might be inappropriate. Click to display it.

[Michael Niedermayer]

Post by Kieran Kunhya
Merry Christmas!
---
libavfilter/vf_scale.c | 11 +++++++++++
1 file changed, 11 insertions(+)

applied

merry Christmess

thanks

[...]