On Sat, Jan 26, 2013 at 1:25 PM, Diego Biurrun <[email protected]> wrote:
> On Sat, Jan 26, 2013 at 01:01:09PM -0500, Daniel Kang wrote:
>> On Sat, Jan 26, 2013 at 3:23 AM, Diego Biurrun <[email protected]> wrote:
>> > On Sat, Jan 26, 2013 at 12:32:16AM -0500, Daniel Kang wrote:
>> >> --- a/libavcodec/x86/dsputil.asm
>> >> +++ b/libavcodec/x86/dsputil.asm
>> >> @@ -879,3 +884,984 @@ cglobal avg_pixels16, 4,5,4
>> >> +; mpeg4 qpel
>> >> +
>> >> +%macro MPEG4_QPEL16_H_LOWPASS 1
>> >> +cglobal %1_mpeg4_qpel16_h_lowpass, 5, 5, 0, 8
>> >
>> > So it seems like dsputil.asm is becoming the new dumping ground for
>> > functions of all kind. It doubles in size after your patch and at
>> > around 2k lines it starts to work against our current efforts of
>> > splitting dsputil into sensibly-sized pieces. If you continue your
>> > porting efforts, it will probably end up around 5k lines or so.
>> >
>> > Whenever there is an opportunity to make dsputil less monolithic comes
>> > up, we should exploit it. That seems to be the case here.
>>
>> I was trying to avoid drama and bikeshedding re: file names and save
>> that for another patch. I guess I could split it in this patch if you
>> want.
>
> Come on, don't blow the issue out of proportion. Just come up with a
> suitable name, maybe ask one or two other people that know the code
> for suitable suggestions. My suggestion would be mpeg4qpel.asm, maybe
> h263qpel.asm, but the former is probably more fitting, not sure.
>
> Even in case you should get three different suggestions and change to one
> after the other, it's easy enough with git and will not hinder your
> workflow at all.
>
> However, going back and forth after your patch has been pushed just
> creates unnecessary churn and annoyance.
Very well, moved to mpeg4qpel.asm
>> >> +%macro QPEL_V_LOW 5
>> >> + paddw m0, m1
>> >> + mova m4, [pw_20]
>> >> + pmullw m4, m0
>> >> + mova m0, %4
>> >> + mova m5, %1
>> >> + paddw m5, m0
>> >> + psubw m4, m5
>> >> + mova m5, %2
>> >> + mova m6, %3
>> >> + paddw m5, m3
>> >> + paddw m6, m2
>> >> + paddw m6, m6
>> >> + psubw m5, m6
>> >> + pmullw m5, [pw_3]
>> >> + paddw m4, [PW_ROUND]
>> >> + paddw m5, m4
>> >> + psraw m5, 5
>> >> + packuswb m5, m5
>> >> + OP_MOV %5, m5, m7
>> >> + SWAP 0,1,2,3
>> >> +%endmacro
>> >
>> > nit: SWAP is not special, format its arguments like the rest of the
>> > macro instructions.
>>
>> I disagree on this one, I think SWAP is special.
>
> The rest of the codebase disagrees with you then. In the rest of the
> files SWAP has spaces after comma and arguments aligned with the other
> instructions.
Only some of it does, but changed.
>> >> --- a/libavcodec/x86/dsputil_avg_template.c
>> >> +++ b/libavcodec/x86/dsputil_avg_template.c
>> >> @@ -24,781 +24,32 @@
>> >> //FIXME the following could be optimized too ...
>> >> +static void DEF(ff_put_no_rnd_pixels16_x2)(uint8_t *block, const uint8_t
>> >> *pixels, int line_size, int h){
>> >> + DEF(ff_put_no_rnd_pixels8_x2)(block , pixels , line_size, h);
>> >> + DEF(ff_put_no_rnd_pixels8_x2)(block+8, pixels+8, line_size, h);
>> >> }
>> >> +static void DEF(ff_put_pixels16_y2)(uint8_t *block, const uint8_t
>> >> *pixels, int line_size, int h){
>> >> + DEF(ff_put_pixels8_y2)(block , pixels , line_size, h);
>> >> + DEF(ff_put_pixels8_y2)(block+8, pixels+8, line_size, h);
>> >> }
>> >> +static void DEF(ff_put_no_rnd_pixels16_y2)(uint8_t *block, const uint8_t
>> >> *pixels, int line_size, int h){
>> >> + DEF(ff_put_no_rnd_pixels8_y2)(block , pixels , line_size, h);
>> >> + DEF(ff_put_no_rnd_pixels8_y2)(block+8, pixels+8, line_size, h);
>> >> }
>> >> +static void DEF(ff_avg_pixels16)(uint8_t *block, const uint8_t *pixels,
>> >> int line_size, int h){
>> >> + DEF(ff_avg_pixels8)(block , pixels , line_size, h);
>> >> + DEF(ff_avg_pixels8)(block+8, pixels+8, line_size, h);
>> >> }
>> >> +static void DEF(ff_avg_pixels16_x2)(uint8_t *block, const uint8_t
>> >> *pixels, int line_size, int h){
>> >> + DEF(ff_avg_pixels8_x2)(block , pixels , line_size, h);
>> >> + DEF(ff_avg_pixels8_x2)(block+8, pixels+8, line_size, h);
>> >> }
>> >> +static void DEF(ff_avg_pixels16_y2)(uint8_t *block, const uint8_t
>> >> *pixels, int line_size, int h){
>> >> + DEF(ff_avg_pixels8_y2)(block , pixels , line_size, h);
>> >> + DEF(ff_avg_pixels8_y2)(block+8, pixels+8, line_size, h);
>> >> }
>> >> +static void DEF(ff_avg_pixels16_xy2)(uint8_t *block, const uint8_t
>> >> *pixels, int line_size, int h){
>> >> + DEF(ff_avg_pixels8_xy2)(block , pixels , line_size, h);
>> >> + DEF(ff_avg_pixels8_xy2)(block+8, pixels+8, line_size, h);
>> >> }
>> >
>> > If you feel motivated, you could fix the formatting as you are changing
>> > all lines anyway.
>>
>> Fixed.
>
> Hehe, sort of :)
>
> Try running the following (GNU) sed command on your tree:
>
> sed -i -e 's/+/ + /g' -e 's/ ,/, /g'
> libavcodec/x86/dsputil_avg_template.c
>
> That should prettyprint it nicely.
Fixed.
>> >> --- a/libavcodec/x86/dsputil_mmx.c
>> >> +++ b/libavcodec/x86/dsputil_mmx.c
>> >> @@ -80,6 +80,143 @@ DECLARE_ALIGNED(16, const xmm_reg, ff_pb_FE) = {
>> >> 0xFEFEFEFEFEFEFEFEULL, 0xFEF
>> >> +
>> >> +/***********************************/
>> >> +/* 3Dnow specific */
>> >> +
>> >> +#define DEF(x) x ## _3dnow
>> >> +
>> >> +#include "dsputil_avg_template.c"
>> >> +
>> >> +#undef DEF
>> >> +
>> >> +/***********************************/
>> >> +/* MMXEXT specific */
>> >> +
>> >> +#define DEF(x) x ## _mmxext
>> >> +
>> >> +#include "dsputil_avg_template.c"
>> >> +
>> >> +#undef DEF
>> >> +
>> >> +#endif /* HAVE_YASM */
>> >
>> > Please keep these blocks where they are for now to make the patch
>> > more readable. We can move them around later.
>>
>> Fixed.
>
> The template inclusion appears to get moved around still...
Oops, fixed.
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
