vp8dsp: Clarify the first dimension of the mc function tables

Message ID 1467398868-48513-1-git-send-email-martin@martin.st
State Committed
Headers show

Commit Message

Martin Storsjö July 1, 2016, 6:47 p.m.
Index 0 is 16x16, 1 is 8x8, 2 is 4x4.
---
 libavcodec/vp8dsp.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Luca Barbato July 1, 2016, 7:24 p.m. | #1
On 01/07/16 20:47, Martin Storsjö wrote:
> Index 0 is 16x16, 1 is 8x8, 2 is 4x4.
> ---
>  libavcodec/vp8dsp.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/vp8dsp.h b/libavcodec/vp8dsp.h
> index 4864cf7..b8a30f9 100644
> --- a/libavcodec/vp8dsp.h
> +++ b/libavcodec/vp8dsp.h
> @@ -70,12 +70,12 @@ typedef struct VP8DSPContext {
>      void (*vp8_h_loop_filter_simple)(uint8_t *dst, ptrdiff_t stride, int flim);
>  
>      /**
> -     * first dimension: width>>3, height is assumed equal to width
> +     * first dimension: 2-(width>>3), height is assumed equal to width
>       * second dimension: 0 if no vertical interpolation is needed;
>       *                   1 4-tap vertical interpolation filter (my & 1)
>       *                   2 6-tap vertical interpolation filter (!(my & 1))
>       * third dimension: same as second dimension, for horizontal interpolation
> -     * so something like put_vp8_epel_pixels_tab[width>>3][2*!!my-(my&1)][2*!!mx-(mx&1)](..., mx, my)
> +     * so something like put_vp8_epel_pixels_tab[2-(width>>3)][2*!!my-(my&1)][2*!!mx-(mx&1)](..., mx, my)
>       */
>      vp8_mc_func put_vp8_epel_pixels_tab[3][3][3];
>      vp8_mc_func put_vp8_bilinear_pixels_tab[3][3][3];
> 

Sure!
Martin Storsjö July 5, 2016, 9:58 a.m. | #2
On Fri, 1 Jul 2016, Martin Storsjö wrote:

> Index 0 is 16x16, 1 is 8x8, 2 is 4x4.
> ---
> libavcodec/vp8dsp.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/vp8dsp.h b/libavcodec/vp8dsp.h
> index 4864cf7..b8a30f9 100644
> --- a/libavcodec/vp8dsp.h
> +++ b/libavcodec/vp8dsp.h
> @@ -70,12 +70,12 @@ typedef struct VP8DSPContext {
>     void (*vp8_h_loop_filter_simple)(uint8_t *dst, ptrdiff_t stride, int flim);
>
>     /**
> -     * first dimension: width>>3, height is assumed equal to width
> +     * first dimension: 2-(width>>3), height is assumed equal to width

FWIW; 2-(width>>3) might give the right values, but isn't technically 
quite right either. The right expression would be 4-log2(width) (or 
4-av_ctz(width)) - should we go with that instead?

// Martin
Ronald Bultje July 5, 2016, 10:36 a.m. | #3
Hi,

On Tue, Jul 5, 2016 at 5:58 AM, Martin Storsjö <martin@martin.st> wrote:

> On Fri, 1 Jul 2016, Martin Storsjö wrote:
>
> Index 0 is 16x16, 1 is 8x8, 2 is 4x4.
>> ---
>> libavcodec/vp8dsp.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/vp8dsp.h b/libavcodec/vp8dsp.h
>> index 4864cf7..b8a30f9 100644
>> --- a/libavcodec/vp8dsp.h
>> +++ b/libavcodec/vp8dsp.h
>> @@ -70,12 +70,12 @@ typedef struct VP8DSPContext {
>>     void (*vp8_h_loop_filter_simple)(uint8_t *dst, ptrdiff_t stride, int
>> flim);
>>
>>     /**
>> -     * first dimension: width>>3, height is assumed equal to width
>> +     * first dimension: 2-(width>>3), height is assumed equal to width
>>
>
> FWIW; 2-(width>>3) might give the right values, but isn't technically
> quite right either. The right expression would be 4-log2(width) (or
> 4-av_ctz(width)) - should we go with that instead?


Since it's documentation, I'd go with 4-log2(width).

Ronald
Ronald Bultje July 5, 2016, 10:39 a.m. | #4
Hi,

one more thing...

On Tue, Jul 5, 2016 at 6:36 AM, Ronald S. Bultje <rsbultje@gmail.com> wrote:

> On Tue, Jul 5, 2016 at 5:58 AM, Martin Storsjö <martin@martin.st> wrote:
>
>> On Fri, 1 Jul 2016, Martin Storsjö wrote:
>>
>>> Index 0 is 16x16, 1 is 8x8, 2 is 4x4.
>>
>>
This isn't accurate either - since we're nitpicking over technical
correctness. vp8_mc_func has a height argument, so height is not part of
the index, only width. index 0: w=16, index 1: w=8, index 2: w=4. Since
this is apparently dubious, it may help to add this (within braces or so)
as additional information to the documentation if you think it helps.

Ronald
Luca Barbato July 5, 2016, 6:01 p.m. | #5
On 05/07/16 11:58, Martin Storsjö wrote:
> On Fri, 1 Jul 2016, Martin Storsjö wrote:
> 
>> Index 0 is 16x16, 1 is 8x8, 2 is 4x4.
>> ---
>> libavcodec/vp8dsp.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/vp8dsp.h b/libavcodec/vp8dsp.h
>> index 4864cf7..b8a30f9 100644
>> --- a/libavcodec/vp8dsp.h
>> +++ b/libavcodec/vp8dsp.h
>> @@ -70,12 +70,12 @@ typedef struct VP8DSPContext {
>>     void (*vp8_h_loop_filter_simple)(uint8_t *dst, ptrdiff_t stride,
>> int flim);
>>
>>     /**
>> -     * first dimension: width>>3, height is assumed equal to width
>> +     * first dimension: 2-(width>>3), height is assumed equal to width
> 
> FWIW; 2-(width>>3) might give the right values, but isn't technically
> quite right either. The right expression would be 4-log2(width) (or
> 4-av_ctz(width)) - should we go with that instead?
> 

Sure.
Martin Storsjö July 7, 2016, 10:25 a.m. | #6
On Tue, 5 Jul 2016, Ronald S. Bultje wrote:

> Hi,
>
> one more thing...
>
> On Tue, Jul 5, 2016 at 6:36 AM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
>
>> On Tue, Jul 5, 2016 at 5:58 AM, Martin Storsjö <martin@martin.st> wrote:
>>
>>> On Fri, 1 Jul 2016, Martin Storsjö wrote:
>>>
>>>> Index 0 is 16x16, 1 is 8x8, 2 is 4x4.
>>>
>>>
> This isn't accurate either - since we're nitpicking over technical
> correctness. vp8_mc_func has a height argument, so height is not part of
> the index, only width. index 0: w=16, index 1: w=8, index 2: w=4. Since
> this is apparently dubious, it may help to add this (within braces or so)
> as additional information to the documentation if you think it helps.

Well, actually, the same comment says "height is assumed equal to width". 
I'll refrain from changing that ATM, but pushed with the commit message 
changed to only say w=16 instead of 16x16, and with 4-log2(width).

// Martin

Patch

diff --git a/libavcodec/vp8dsp.h b/libavcodec/vp8dsp.h
index 4864cf7..b8a30f9 100644
--- a/libavcodec/vp8dsp.h
+++ b/libavcodec/vp8dsp.h
@@ -70,12 +70,12 @@  typedef struct VP8DSPContext {
     void (*vp8_h_loop_filter_simple)(uint8_t *dst, ptrdiff_t stride, int flim);
 
     /**
-     * first dimension: width>>3, height is assumed equal to width
+     * first dimension: 2-(width>>3), height is assumed equal to width
      * second dimension: 0 if no vertical interpolation is needed;
      *                   1 4-tap vertical interpolation filter (my & 1)
      *                   2 6-tap vertical interpolation filter (!(my & 1))
      * third dimension: same as second dimension, for horizontal interpolation
-     * so something like put_vp8_epel_pixels_tab[width>>3][2*!!my-(my&1)][2*!!mx-(mx&1)](..., mx, my)
+     * so something like put_vp8_epel_pixels_tab[2-(width>>3)][2*!!my-(my&1)][2*!!mx-(mx&1)](..., mx, my)
      */
     vp8_mc_func put_vp8_epel_pixels_tab[3][3][3];
     vp8_mc_func put_vp8_bilinear_pixels_tab[3][3][3];