On Wed, Mar 26, 2014 at 7:26 PM, Diego Biurrun <[email protected]> wrote:
> On Wed, Mar 26, 2014 at 05:48:53PM +0100, Vittorio Giovara wrote:
>> --- a/configure
>> +++ b/configure
>> @@ -1846,6 +1846,7 @@ vp5_decoder_select="h264chroma hpeldsp videodsp vp3dsp"
>> vp6_decoder_select="h264chroma hpeldsp huffman videodsp vp3dsp"
>> vp6a_decoder_select="vp6_decoder"
>> vp6f_decoder_select="vp6_decoder"
>> +vp7_decoder_select="h264pred videodsp"
>> vp8_decoder_select="h264pred videodsp"
>
> How much code is actually shared? It might make sense to have one decoder
> select the other.
A lot, VP7 is basically a subset of VP8 and some bug^Wfeatures
>> --- a/libavcodec/vp8dsp.c
>> +++ b/libavcodec/vp8dsp.c
>> @@ -348,7 +482,7 @@ PUT_PIXELS(4)
>> cm[(F[2] * src[x + 0 * stride] - F[1] * src[x - 1 * stride] +
>> \
>> F[3] * src[x + 1 * stride] - F[4] * src[x + 2 * stride] + 64) >> 7]
>>
>> -#define VP8_EPEL_H(SIZE, TAPS)
>> \
>> +#define VP8_EPEL_H(SIZE, TAPS) \
>> static void put_vp8_epel ## SIZE ## _h ## TAPS ## _c(uint8_t *dst,
>> \
>> ptrdiff_t
>> dststride, \
>> uint8_t *src,
>> \
>> @@ -397,7 +531,6 @@ PUT_PIXELS(4)
>> uint8_t tmp_array[(2 * SIZE + VTAPS - 1) * SIZE];
>> \
>> uint8_t *tmp = tmp_array;
>> \
>> src -= (2 - (VTAPS == 4)) * srcstride;
>> \
>> -
>> \
>> for (y = 0; y < h + VTAPS - 1; y++) {
>> \
>> for (x = 0; x < SIZE; x++)
>> \
>> tmp[x] = FILTER_ ## HTAPS ## TAP(src, filter, 1);
>> \
>> @@ -406,7 +539,6 @@ PUT_PIXELS(4)
>> }
>> \
>> tmp = tmp_array + (2 - (VTAPS == 4)) * SIZE;
>> \
>> filter = subpel_filters[my - 1];
>> \
>> -
>> \
>> for (y = 0; y < h; y++) {
>> \
>> for (x = 0; x < SIZE; x++)
>> \
>> dst[x] = FILTER_ ## VTAPS ## TAP(tmp, filter, SIZE);
>> \
>> @@ -441,7 +573,7 @@ VP8_EPEL_HV(16, 6, 6)
>> VP8_EPEL_HV( 8, 6, 6)
>> VP8_EPEL_HV( 4, 6, 6)
>>
>> -#define VP8_BILINEAR(SIZE)
>> \
>> +#define VP8_BILINEAR(SIZE) \
>> static void put_vp8_bilinear ## SIZE ## _h_c(uint8_t *dst, ptrdiff_t
>> dstride, \
>> uint8_t *src, ptrdiff_t
>> sstride, \
>> int h, int mx, int my)
>> \
>> @@ -496,7 +628,7 @@ VP8_BILINEAR(16)
>> VP8_BILINEAR(8)
>> VP8_BILINEAR(4)
>>
>> -#define VP8_MC_FUNC(IDX, SIZE) \
>> +#define VP8_MC_FUNC(IDX, SIZE)
>> \
>> dsp->put_vp8_epel_pixels_tab[IDX][0][0] = put_vp8_pixels ## SIZE ## _c;
>> \
>> dsp->put_vp8_epel_pixels_tab[IDX][0][1] = put_vp8_epel ## SIZE ##
>> _h4_c; \
>> dsp->put_vp8_epel_pixels_tab[IDX][0][2] = put_vp8_epel ## SIZE ##
>> _h6_c; \
>> @@ -507,7 +639,7 @@ VP8_BILINEAR(4)
>> dsp->put_vp8_epel_pixels_tab[IDX][2][1] = put_vp8_epel ## SIZE ##
>> _h4v6_c; \
>> dsp->put_vp8_epel_pixels_tab[IDX][2][2] = put_vp8_epel ## SIZE ##
>> _h6v6_c
>>
>> -#define VP8_BILINEAR_MC_FUNC(IDX, SIZE) \
>> +#define VP8_BILINEAR_MC_FUNC(IDX, SIZE)
>> \
>> dsp->put_vp8_bilinear_pixels_tab[IDX][0][0] = put_vp8_pixels ## SIZE ##
>> _c; \
>> dsp->put_vp8_bilinear_pixels_tab[IDX][0][1] = put_vp8_bilinear ## SIZE
>> ## _h_c; \
>> dsp->put_vp8_bilinear_pixels_tab[IDX][0][2] = put_vp8_bilinear ## SIZE
>> ## _h_c; \
>
> Please self-review for stray changes.
This is a result of a bad fixup sorry.
>
>> @@ -518,27 +650,35 @@ VP8_BILINEAR(4)
>>
>> -av_cold void ff_vp8dsp_init(VP8DSPContext *dsp)
>> +av_cold void ff_vp8dsp_init(VP8DSPContext *dsp, int vp7)
>
> Why don't you split the dsp stuff in two and call just one or both of
> ff_vp8dsp_init() and ff_vp8dsp_init()?
I'm not sure I understand. You'd prefer a single separate
ff_vp7dsp_init() and ff_vp8dsp_init() or a single ff_vp78dsp_init()
that calls the relevant parts of the dsp?
Vittorio
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
