diff mbox

[FFmpeg-devel] Revert "Merge commit '0a39c9ac0bfd7345fe676b4e2707d9cec3cbb553'"

Message ID 20170131193442.11695-1-michael@niedermayer.cc
State Accepted
Commit 536ac72f46b7b5094949b4e6a7e07cc8de86aac9
Headers show

Commit Message

Michael Niedermayer Jan. 31, 2017, 7:34 p.m. UTC 
The assumtation this is based on is wrong, the code is not always run with bitexact flags

This reverts commit a956164e1eb3418922cae949f02ad4035f013213, reversing
changes made to f6005907fdeb9e4de37568ed5c1a8e7b869126f6.
---
 libavcodec/x86/hpeldsp.h          |  2 +-
 libavcodec/x86/hpeldsp_init.c     |  2 +-
 libavcodec/x86/hpeldsp_vp3_init.c | 14 +++++++++-----
 3 files changed, 11 insertions(+), 7 deletions(-)

Comments

James Almer Jan. 31, 2017, 8:39 p.m. UTC | #1 
On 1/31/2017 4:34 PM, Michael Niedermayer wrote:
> The assumtation this is based on is wrong, the code is not always run with bitexact flags
> 
> This reverts commit a956164e1eb3418922cae949f02ad4035f013213, reversing
> changes made to f6005907fdeb9e4de37568ed5c1a8e7b869126f6.
> ---
>  libavcodec/x86/hpeldsp.h          |  2 +-
>  libavcodec/x86/hpeldsp_init.c     |  2 +-
>  libavcodec/x86/hpeldsp_vp3_init.c | 14 +++++++++-----
>  3 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/x86/hpeldsp.h b/libavcodec/x86/hpeldsp.h
> index 0ecc97a83a..bf97029b57 100644
> --- a/libavcodec/x86/hpeldsp.h
> +++ b/libavcodec/x86/hpeldsp.h
> @@ -52,6 +52,6 @@ void ff_put_pixels16_xy2_sse2(uint8_t *block, const uint8_t *pixels,
>  void ff_put_pixels16_xy2_ssse3(uint8_t *block, const uint8_t *pixels,
>                                 ptrdiff_t line_size, int h);
>  
> -void ff_hpeldsp_vp3_init_x86(HpelDSPContext *c, int cpu_flags);
> +void ff_hpeldsp_vp3_init_x86(HpelDSPContext *c, int cpu_flags, int flags);
>  
>  #endif /* AVCODEC_X86_HPELDSP_H */
> diff --git a/libavcodec/x86/hpeldsp_init.c b/libavcodec/x86/hpeldsp_init.c
> index e583bd9ffe..58e27e3542 100644
> --- a/libavcodec/x86/hpeldsp_init.c
> +++ b/libavcodec/x86/hpeldsp_init.c
> @@ -309,5 +309,5 @@ av_cold void ff_hpeldsp_init_x86(HpelDSPContext *c, int flags)
>          hpeldsp_init_ssse3(c, flags);
>  
>      if (CONFIG_VP3_DECODER)

How about checking for AV_CODEC_FLAG_BITEXACT here instead of reverting the
function signature? Keeps differences as minimal as possible while having
the same effect.

> -        ff_hpeldsp_vp3_init_x86(c, cpu_flags);
> +        ff_hpeldsp_vp3_init_x86(c, cpu_flags, flags);
>  }
> diff --git a/libavcodec/x86/hpeldsp_vp3_init.c b/libavcodec/x86/hpeldsp_vp3_init.c
> index 17fdd081f3..5979f4123c 100644
> --- a/libavcodec/x86/hpeldsp_vp3_init.c
> +++ b/libavcodec/x86/hpeldsp_vp3_init.c
> @@ -38,15 +38,19 @@ void ff_put_no_rnd_pixels8_y2_exact_3dnow(uint8_t *block,
>                                            const uint8_t *pixels,
>                                            ptrdiff_t line_size, int h);
>  
> -av_cold void ff_hpeldsp_vp3_init_x86(HpelDSPContext *c, int cpu_flags)
> +av_cold void ff_hpeldsp_vp3_init_x86(HpelDSPContext *c, int cpu_flags, int flags)
>  {
>      if (EXTERNAL_AMD3DNOW(cpu_flags)) {
> -        c->put_no_rnd_pixels_tab[1][1] = ff_put_no_rnd_pixels8_x2_exact_3dnow;
> -        c->put_no_rnd_pixels_tab[1][2] = ff_put_no_rnd_pixels8_y2_exact_3dnow;
> +        if (flags & AV_CODEC_FLAG_BITEXACT) {
> +            c->put_no_rnd_pixels_tab[1][1] = ff_put_no_rnd_pixels8_x2_exact_3dnow;
> +            c->put_no_rnd_pixels_tab[1][2] = ff_put_no_rnd_pixels8_y2_exact_3dnow;
> +        }
>      }
>  
>      if (EXTERNAL_MMXEXT(cpu_flags)) {
> -        c->put_no_rnd_pixels_tab[1][1] = ff_put_no_rnd_pixels8_x2_exact_mmxext;
> -        c->put_no_rnd_pixels_tab[1][2] = ff_put_no_rnd_pixels8_y2_exact_mmxext;
> +        if (flags & AV_CODEC_FLAG_BITEXACT) {
> +            c->put_no_rnd_pixels_tab[1][1] = ff_put_no_rnd_pixels8_x2_exact_mmxext;
> +            c->put_no_rnd_pixels_tab[1][2] = ff_put_no_rnd_pixels8_y2_exact_mmxext;
> +        }
>      }
>  }
>
Michael Niedermayer Jan. 31, 2017, 10:12 p.m. UTC | #2 
On Tue, Jan 31, 2017 at 05:39:56PM -0300, James Almer wrote:
> On 1/31/2017 4:34 PM, Michael Niedermayer wrote:
> > The assumtation this is based on is wrong, the code is not always run with bitexact flags
> > 
> > This reverts commit a956164e1eb3418922cae949f02ad4035f013213, reversing
> > changes made to f6005907fdeb9e4de37568ed5c1a8e7b869126f6.
> > ---
> >  libavcodec/x86/hpeldsp.h          |  2 +-
> >  libavcodec/x86/hpeldsp_init.c     |  2 +-
> >  libavcodec/x86/hpeldsp_vp3_init.c | 14 +++++++++-----
> >  3 files changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/libavcodec/x86/hpeldsp.h b/libavcodec/x86/hpeldsp.h
> > index 0ecc97a83a..bf97029b57 100644
> > --- a/libavcodec/x86/hpeldsp.h
> > +++ b/libavcodec/x86/hpeldsp.h
> > @@ -52,6 +52,6 @@ void ff_put_pixels16_xy2_sse2(uint8_t *block, const uint8_t *pixels,
> >  void ff_put_pixels16_xy2_ssse3(uint8_t *block, const uint8_t *pixels,
> >                                 ptrdiff_t line_size, int h);
> >  
> > -void ff_hpeldsp_vp3_init_x86(HpelDSPContext *c, int cpu_flags);
> > +void ff_hpeldsp_vp3_init_x86(HpelDSPContext *c, int cpu_flags, int flags);
> >  
> >  #endif /* AVCODEC_X86_HPELDSP_H */
> > diff --git a/libavcodec/x86/hpeldsp_init.c b/libavcodec/x86/hpeldsp_init.c
> > index e583bd9ffe..58e27e3542 100644
> > --- a/libavcodec/x86/hpeldsp_init.c
> > +++ b/libavcodec/x86/hpeldsp_init.c
> > @@ -309,5 +309,5 @@ av_cold void ff_hpeldsp_init_x86(HpelDSPContext *c, int flags)
> >          hpeldsp_init_ssse3(c, flags);
> >  
> >      if (CONFIG_VP3_DECODER)
> 
> How about checking for AV_CODEC_FLAG_BITEXACT here instead of reverting the
> function signature? Keeps differences as minimal as possible while having
> the same effect.

technically yes thats possible it makes the code confusing though and
may cause bugs in the future, for example then the whole set of
vp3 optimizations here would depend on AV_CODEC_FLAG_BITEXACT being set
and thats unexpected. Unexpected things / surprises can be bad for code
quality.

Anyway i can change it to that test it and resubmit if preferred ?


[...]
James Almer Jan. 31, 2017, 11:27 p.m. UTC | #3 
On 1/31/2017 7:12 PM, Michael Niedermayer wrote:
> On Tue, Jan 31, 2017 at 05:39:56PM -0300, James Almer wrote:
>> On 1/31/2017 4:34 PM, Michael Niedermayer wrote:
>>> The assumtation this is based on is wrong, the code is not always run with bitexact flags

Assumption not assumtation. Missed it the first time.

>>>
>>> This reverts commit a956164e1eb3418922cae949f02ad4035f013213, reversing
>>> changes made to f6005907fdeb9e4de37568ed5c1a8e7b869126f6.
>>> ---
>>>  libavcodec/x86/hpeldsp.h          |  2 +-
>>>  libavcodec/x86/hpeldsp_init.c     |  2 +-
>>>  libavcodec/x86/hpeldsp_vp3_init.c | 14 +++++++++-----
>>>  3 files changed, 11 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/libavcodec/x86/hpeldsp.h b/libavcodec/x86/hpeldsp.h
>>> index 0ecc97a83a..bf97029b57 100644
>>> --- a/libavcodec/x86/hpeldsp.h
>>> +++ b/libavcodec/x86/hpeldsp.h
>>> @@ -52,6 +52,6 @@ void ff_put_pixels16_xy2_sse2(uint8_t *block, const uint8_t *pixels,
>>>  void ff_put_pixels16_xy2_ssse3(uint8_t *block, const uint8_t *pixels,
>>>                                 ptrdiff_t line_size, int h);
>>>  
>>> -void ff_hpeldsp_vp3_init_x86(HpelDSPContext *c, int cpu_flags);
>>> +void ff_hpeldsp_vp3_init_x86(HpelDSPContext *c, int cpu_flags, int flags);
>>>  
>>>  #endif /* AVCODEC_X86_HPELDSP_H */
>>> diff --git a/libavcodec/x86/hpeldsp_init.c b/libavcodec/x86/hpeldsp_init.c
>>> index e583bd9ffe..58e27e3542 100644
>>> --- a/libavcodec/x86/hpeldsp_init.c
>>> +++ b/libavcodec/x86/hpeldsp_init.c
>>> @@ -309,5 +309,5 @@ av_cold void ff_hpeldsp_init_x86(HpelDSPContext *c, int flags)
>>>          hpeldsp_init_ssse3(c, flags);
>>>  
>>>      if (CONFIG_VP3_DECODER)
>>
>> How about checking for AV_CODEC_FLAG_BITEXACT here instead of reverting the
>> function signature? Keeps differences as minimal as possible while having
>> the same effect.
> 
> technically yes thats possible it makes the code confusing though and
> may cause bugs in the future, for example then the whole set of
> vp3 optimizations here would depend on AV_CODEC_FLAG_BITEXACT being set
> and thats unexpected. Unexpected things / surprises can be bad for code
> quality.
> 
> Anyway i can change it to that test it and resubmit if preferred ?

No, it was mostly a nit. The patch is ok as is. The differences with libav
with this change will be minimal in any case.
Michael Niedermayer Feb. 1, 2017, 1 a.m. UTC | #4 
On Tue, Jan 31, 2017 at 08:27:41PM -0300, James Almer wrote:
> On 1/31/2017 7:12 PM, Michael Niedermayer wrote:
> > On Tue, Jan 31, 2017 at 05:39:56PM -0300, James Almer wrote:
> >> On 1/31/2017 4:34 PM, Michael Niedermayer wrote:
> >>> The assumtation this is based on is wrong, the code is not always run with bitexact flags
> 
> Assumption not assumtation. Missed it the first time.

corrected

> 
> >>>
> >>> This reverts commit a956164e1eb3418922cae949f02ad4035f013213, reversing
> >>> changes made to f6005907fdeb9e4de37568ed5c1a8e7b869126f6.
> >>> ---
> >>>  libavcodec/x86/hpeldsp.h          |  2 +-
> >>>  libavcodec/x86/hpeldsp_init.c     |  2 +-
> >>>  libavcodec/x86/hpeldsp_vp3_init.c | 14 +++++++++-----
> >>>  3 files changed, 11 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/libavcodec/x86/hpeldsp.h b/libavcodec/x86/hpeldsp.h
> >>> index 0ecc97a83a..bf97029b57 100644
> >>> --- a/libavcodec/x86/hpeldsp.h
> >>> +++ b/libavcodec/x86/hpeldsp.h
> >>> @@ -52,6 +52,6 @@ void ff_put_pixels16_xy2_sse2(uint8_t *block, const uint8_t *pixels,
> >>>  void ff_put_pixels16_xy2_ssse3(uint8_t *block, const uint8_t *pixels,
> >>>                                 ptrdiff_t line_size, int h);
> >>>  
> >>> -void ff_hpeldsp_vp3_init_x86(HpelDSPContext *c, int cpu_flags);
> >>> +void ff_hpeldsp_vp3_init_x86(HpelDSPContext *c, int cpu_flags, int flags);
> >>>  
> >>>  #endif /* AVCODEC_X86_HPELDSP_H */
> >>> diff --git a/libavcodec/x86/hpeldsp_init.c b/libavcodec/x86/hpeldsp_init.c
> >>> index e583bd9ffe..58e27e3542 100644
> >>> --- a/libavcodec/x86/hpeldsp_init.c
> >>> +++ b/libavcodec/x86/hpeldsp_init.c
> >>> @@ -309,5 +309,5 @@ av_cold void ff_hpeldsp_init_x86(HpelDSPContext *c, int flags)
> >>>          hpeldsp_init_ssse3(c, flags);
> >>>  
> >>>      if (CONFIG_VP3_DECODER)
> >>
> >> How about checking for AV_CODEC_FLAG_BITEXACT here instead of reverting the
> >> function signature? Keeps differences as minimal as possible while having
> >> the same effect.
> > 
> > technically yes thats possible it makes the code confusing though and
> > may cause bugs in the future, for example then the whole set of
> > vp3 optimizations here would depend on AV_CODEC_FLAG_BITEXACT being set
> > and thats unexpected. Unexpected things / surprises can be bad for code
> > quality.
> > 
> > Anyway i can change it to that test it and resubmit if preferred ?
> 
> No, it was mostly a nit. The patch is ok as is. The differences with libav
> with this change will be minimal in any case.

applied

thanks

[...]
diff mbox

Patch

diff --git a/libavcodec/x86/hpeldsp.h b/libavcodec/x86/hpeldsp.h
index 0ecc97a83a..bf97029b57 100644
--- a/libavcodec/x86/hpeldsp.h
+++ b/libavcodec/x86/hpeldsp.h
@@ -52,6 +52,6 @@  void ff_put_pixels16_xy2_sse2(uint8_t *block, const uint8_t *pixels,
 void ff_put_pixels16_xy2_ssse3(uint8_t *block, const uint8_t *pixels,
                                ptrdiff_t line_size, int h);
 
-void ff_hpeldsp_vp3_init_x86(HpelDSPContext *c, int cpu_flags);
+void ff_hpeldsp_vp3_init_x86(HpelDSPContext *c, int cpu_flags, int flags);
 
 #endif /* AVCODEC_X86_HPELDSP_H */
diff --git a/libavcodec/x86/hpeldsp_init.c b/libavcodec/x86/hpeldsp_init.c
index e583bd9ffe..58e27e3542 100644
--- a/libavcodec/x86/hpeldsp_init.c
+++ b/libavcodec/x86/hpeldsp_init.c
@@ -309,5 +309,5 @@  av_cold void ff_hpeldsp_init_x86(HpelDSPContext *c, int flags)
         hpeldsp_init_ssse3(c, flags);
 
     if (CONFIG_VP3_DECODER)
-        ff_hpeldsp_vp3_init_x86(c, cpu_flags);
+        ff_hpeldsp_vp3_init_x86(c, cpu_flags, flags);
 }
diff --git a/libavcodec/x86/hpeldsp_vp3_init.c b/libavcodec/x86/hpeldsp_vp3_init.c
index 17fdd081f3..5979f4123c 100644
--- a/libavcodec/x86/hpeldsp_vp3_init.c
+++ b/libavcodec/x86/hpeldsp_vp3_init.c
@@ -38,15 +38,19 @@  void ff_put_no_rnd_pixels8_y2_exact_3dnow(uint8_t *block,
                                           const uint8_t *pixels,
                                           ptrdiff_t line_size, int h);
 
-av_cold void ff_hpeldsp_vp3_init_x86(HpelDSPContext *c, int cpu_flags)
+av_cold void ff_hpeldsp_vp3_init_x86(HpelDSPContext *c, int cpu_flags, int flags)
 {
     if (EXTERNAL_AMD3DNOW(cpu_flags)) {
-        c->put_no_rnd_pixels_tab[1][1] = ff_put_no_rnd_pixels8_x2_exact_3dnow;
-        c->put_no_rnd_pixels_tab[1][2] = ff_put_no_rnd_pixels8_y2_exact_3dnow;
+        if (flags & AV_CODEC_FLAG_BITEXACT) {
+            c->put_no_rnd_pixels_tab[1][1] = ff_put_no_rnd_pixels8_x2_exact_3dnow;
+            c->put_no_rnd_pixels_tab[1][2] = ff_put_no_rnd_pixels8_y2_exact_3dnow;
+        }
     }
 
     if (EXTERNAL_MMXEXT(cpu_flags)) {
-        c->put_no_rnd_pixels_tab[1][1] = ff_put_no_rnd_pixels8_x2_exact_mmxext;
-        c->put_no_rnd_pixels_tab[1][2] = ff_put_no_rnd_pixels8_y2_exact_mmxext;
+        if (flags & AV_CODEC_FLAG_BITEXACT) {
+            c->put_no_rnd_pixels_tab[1][1] = ff_put_no_rnd_pixels8_x2_exact_mmxext;
+            c->put_no_rnd_pixels_tab[1][2] = ff_put_no_rnd_pixels8_y2_exact_mmxext;
+        }
     }
 }