Re: [FFmpeg-devel] [PATCH] avcodec/vc1_pred: properly clip interlaced motion vectors

2018-04-21 Thread Carl Eugen Hoyos 
2018-04-21 21:05 GMT+02:00, Jerome Borsboom :
>> Fixes #2557.
>>
>> Signed-off-by: Paul B Mahol 
>> ---
>>  libavcodec/vc1_pred.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/vc1_pred.c b/libavcodec/vc1_pred.c
>> index 54712f6b7a..9f42a930fe 100644
>> --- a/libavcodec/vc1_pred.c
>> +++ b/libavcodec/vc1_pred.c
>> @@ -98,9 +98,9 @@ static av_always_inline int scaleforsame_y(VC1Context
>> *v, int i, int n /* MV */,
>>  }
>>
>>  if (v->cur_field_type && !v->ref_field_type[dir])
>> -return av_clip(scaledvalue, -v->range_y / 2 + 1, v->range_y / 2);
>> +return av_clip(scaledvalue, -v->range_y / 2 - 1, v->range_y / 2);
>>  else
>> -return av_clip(scaledvalue, -v->range_y / 2, v->range_y / 2 - 1);
>> +return av_clip(scaledvalue, -v->range_y / 2, v->range_y / 2 + 1);
>>  }
>>
>>  static av_always_inline int scaleforopp_x(VC1Context *v, int n /* MV */)
>> --
>> 2.11.0
>
> scaleforsame_y references ref_field_type. Therefore, it needs to be set
> before scaleforsame is called.

Please mention ticket #2557 in the commit message.

> Signed-off-by: Jerome Borsboom 
> ---
> I am not sure your patch is correct. The existing implementation agrees with
> VC-1 spec.
> See Figure 119 in 10.3.5.4.3.4.2 in particular. Please have a look if the
> patch below solves
> the issue. ref_field_type is referenced in scaleforsame_y and needs to be
> set earlier in
> ff_vc1_pred_mv to have the desired effect.

This patch also fixes the issue for the given sample

Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/vc1_pred: properly clip interlaced motion vectors

2018-04-21 Thread Jerome Borsboom 
> Fixes #2557.
> 
> Signed-off-by: Paul B Mahol 
> ---
>  libavcodec/vc1_pred.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/vc1_pred.c b/libavcodec/vc1_pred.c
> index 54712f6b7a..9f42a930fe 100644
> --- a/libavcodec/vc1_pred.c
> +++ b/libavcodec/vc1_pred.c
> @@ -98,9 +98,9 @@ static av_always_inline int scaleforsame_y(VC1Context *v, 
> int i, int n /* MV */,
>  }
>  
>  if (v->cur_field_type && !v->ref_field_type[dir])
> -return av_clip(scaledvalue, -v->range_y / 2 + 1, v->range_y / 2);
> +return av_clip(scaledvalue, -v->range_y / 2 - 1, v->range_y / 2);
>  else
> -return av_clip(scaledvalue, -v->range_y / 2, v->range_y / 2 - 1);
> +return av_clip(scaledvalue, -v->range_y / 2, v->range_y / 2 + 1);
>  }
>  
>  static av_always_inline int scaleforopp_x(VC1Context *v, int n /* MV */)
> -- 
> 2.11.0

scaleforsame_y references ref_field_type. Therefore, it needs to be set
before scaleforsame is called.

Signed-off-by: Jerome Borsboom 
---
I am not sure your patch is correct. The existing implementation agrees with 
VC-1 spec.
See Figure 119 in 10.3.5.4.3.4.2 in particular. Please have a look if the patch 
below solves
the issue. ref_field_type is referenced in scaleforsame_y and needs to be set 
earlier in
ff_vc1_pred_mv to have the desired effect.

 libavcodec/vc1_pred.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libavcodec/vc1_pred.c b/libavcodec/vc1_pred.c
index 54712f6b7a..3a52a22bc6 100644
--- a/libavcodec/vc1_pred.c
+++ b/libavcodec/vc1_pred.c
@@ -341,6 +341,8 @@ void ff_vc1_pred_mv(VC1Context *v, int n, int dmv_x, int 
dmv_y,
 } else
 opposite = 0;
 if (opposite) {
+v->mv_f[dir][xy + v->blocks_off] = 1;
+v->ref_field_type[dir] = !v->cur_field_type;
 if (a_valid && !a_f) {
 field_predA[0] = scaleforopp(v, field_predA[0], 0, dir);
 field_predA[1] = scaleforopp(v, field_predA[1], 1, dir);
@@ -353,9 +355,9 @@ void ff_vc1_pred_mv(VC1Context *v, int n, int dmv_x, int 
dmv_y,
 field_predC[0] = scaleforopp(v, field_predC[0], 0, dir);
 field_predC[1] = scaleforopp(v, field_predC[1], 1, dir);
 }
-v->mv_f[dir][xy + v->blocks_off] = 1;
-v->ref_field_type[dir] = !v->cur_field_type;
 } else {
+v->mv_f[dir][xy + v->blocks_off] = 0;
+v->ref_field_type[dir] = v->cur_field_type;
 if (a_valid && a_f) {
 field_predA[0] = scaleforsame(v, n, field_predA[0], 0, dir);
 field_predA[1] = scaleforsame(v, n, field_predA[1], 1, dir);
@@ -368,8 +370,6 @@ void ff_vc1_pred_mv(VC1Context *v, int n, int dmv_x, int 
dmv_y,
 field_predC[0] = scaleforsame(v, n, field_predC[0], 0, dir);
 field_predC[1] = scaleforsame(v, n, field_predC[1], 1, dir);
 }
-v->mv_f[dir][xy + v->blocks_off] = 0;
-v->ref_field_type[dir] = v->cur_field_type;
 }
 
 if (a_valid) {
-- 
2.13.6


___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/vc1_pred: properly clip interlaced motion vectors

2018-04-21 Thread Paul B Mahol 
On 4/21/18, Carl Eugen Hoyos  wrote:
> 2018-04-21 10:22 GMT+02:00, Paul B Mahol :
>> Fixes #2557.
>>
>> Signed-off-by: Paul B Mahol 
>> ---
>>  libavcodec/vc1_pred.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/vc1_pred.c b/libavcodec/vc1_pred.c
>> index 54712f6b7a..9f42a930fe 100644
>> --- a/libavcodec/vc1_pred.c
>> +++ b/libavcodec/vc1_pred.c
>> @@ -98,9 +98,9 @@ static av_always_inline int scaleforsame_y(VC1Context
>> *v,
>> int i, int n /* MV */,
>>  }
>>
>>  if (v->cur_field_type && !v->ref_field_type[dir])
>> -return av_clip(scaledvalue, -v->range_y / 2 + 1, v->range_y / 2);
>> +return av_clip(scaledvalue, -v->range_y / 2 - 1, v->range_y / 2);
>>  else
>> -return av_clip(scaledvalue, -v->range_y / 2, v->range_y / 2 - 1);
>> +return av_clip(scaledvalue, -v->range_y / 2, v->range_y / 2 + 1);
>
> Makes the affected frames bit-exact, so lgtm.

This probably needs no + 1 and - 1 part, so will commit without it.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/vc1_pred: properly clip interlaced motion vectors

2018-04-21 Thread Carl Eugen Hoyos 
2018-04-21 10:22 GMT+02:00, Paul B Mahol :
> Fixes #2557.
>
> Signed-off-by: Paul B Mahol 
> ---
>  libavcodec/vc1_pred.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/vc1_pred.c b/libavcodec/vc1_pred.c
> index 54712f6b7a..9f42a930fe 100644
> --- a/libavcodec/vc1_pred.c
> +++ b/libavcodec/vc1_pred.c
> @@ -98,9 +98,9 @@ static av_always_inline int scaleforsame_y(VC1Context *v,
> int i, int n /* MV */,
>  }
>
>  if (v->cur_field_type && !v->ref_field_type[dir])
> -return av_clip(scaledvalue, -v->range_y / 2 + 1, v->range_y / 2);
> +return av_clip(scaledvalue, -v->range_y / 2 - 1, v->range_y / 2);
>  else
> -return av_clip(scaledvalue, -v->range_y / 2, v->range_y / 2 - 1);
> +return av_clip(scaledvalue, -v->range_y / 2, v->range_y / 2 + 1);

Makes the affected frames bit-exact, so lgtm.

Thank you, Carl Eugen
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel