[PATCH] Fix function parameters for rgb48 to YV12 functions.
$subj
On Mon, Jan 11, 2010 at 05:21:19AM -0200, Ramiro Polla wrote:
$subj
I'm not a maintainer but first patch looks correct. In any case pass N liters of Trocadero in my direction.
Hi, 2010/1/11 Ramiro Polla <ramiro.polla at gmail.com>:
$subj
Why do we have to use long to replace int? We support larger "width" under 64 bit machines? -- Zuxy Beauty is truth, While truth is beauty. PGP KeyID: E8555ED6
On Mon, Jan 11, 2010 at 6:14 AM, Zuxy Meng <zuxy.meng at gmail.com> wrote:
2010/1/11 Ramiro Polla <ramiro.polla at gmail.com>:
$subj
Why do we have to use long to replace int? We support larger "width" under 64 bit machines?
Have you never had to resize a 5 billion pixels wide image? =) It's because of the optimized assembly functions. If the parameters are long, they can be used directly by both x86_32 and x86_64 asm. But that's an issue for mingw-w64, which has long the same size as int. Maybe the parameters should all be int, and the optimized functions should declare a local x86_reg variable to access, but that might mean one more memory access for them...
On Mon, Jan 11, 2010 at 05:21:19AM -0200, Ramiro Polla wrote:
$subj
swscale.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) cf0e5d1b0d322cac274144c19a6842deaab35f06 0001-Add-missing-parameter-to-rgb48-to-YV12-functions.patch From ddf2d2f6f79556d0b65e2657772f42963b35d602 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla at gmail.com> Date: Mon, 11 Jan 2010 05:00:38 -0200 Subject: [PATCH] Add missing parameter to rgb48 to YV12 functions.
ok [...]
swscale.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 89382350eded6c26d436823cd03024fe8a5b7fdc 0002-Add-const-qualifier-to-rgb48-to-YV12-functions-src.patch From 995f52a73adc9b7b66453d20405cc37094a94314 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla at gmail.com> Date: Mon, 11 Jan 2010 05:02:44 -0200 Subject: [PATCH] Add const qualifier to rgb48 to YV12 functions' src.
ok
swscale.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 104e7c17caec26638ce07bb04dc15eaa0d9133fb 0003-int-long-for-width-parameter-in-rgb48-to-YV12-fun.patch From da0f8f381054f23a32251981354ec56fb1232a56 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla at gmail.com> Date: Mon, 11 Jan 2010 05:04:09 -0200 Subject: [PATCH] int -> long for width parameter in rgb48 to YV12 functions.
dont we have a proper type for that? also these unused vars could need a av_unused [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Observe your enemies, for they first find out your faults. -- Antisthenes
On Mon, Jan 11, 2010 at 12:51 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Mon, Jan 11, 2010 at 05:21:19AM -0200, Ramiro Polla wrote:
$subj
?swscale.c | ? ?9 ++++++--- ?1 file changed, 6 insertions(+), 3 deletions(-) cf0e5d1b0d322cac274144c19a6842deaab35f06 ?0001-Add-missing-parameter-to-rgb48-to-YV12-functions.patch From ddf2d2f6f79556d0b65e2657772f42963b35d602 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla at gmail.com> Date: Mon, 11 Jan 2010 05:00:38 -0200 Subject: [PATCH] Add missing parameter to rgb48 to YV12 functions.
ok
Applied.
?swscale.c | ? ?8 ++++---- ?1 file changed, 4 insertions(+), 4 deletions(-) 89382350eded6c26d436823cd03024fe8a5b7fdc ?0002-Add-const-qualifier-to-rgb48-to-YV12-functions-src.patch From 995f52a73adc9b7b66453d20405cc37094a94314 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla at gmail.com> Date: Mon, 11 Jan 2010 05:02:44 -0200 Subject: [PATCH] Add const qualifier to rgb48 to YV12 functions' src.
ok
Applied.
?swscale.c | ? ?6 +++--- ?1 file changed, 3 insertions(+), 3 deletions(-) 104e7c17caec26638ce07bb04dc15eaa0d9133fb ?0003-int-long-for-width-parameter-in-rgb48-to-YV12-fun.patch From da0f8f381054f23a32251981354ec56fb1232a56 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla at gmail.com> Date: Mon, 11 Jan 2010 05:04:09 -0200 Subject: [PATCH] int -> long for width parameter in rgb48 to YV12 functions.
dont we have a proper type for that?
You mean x86_reg? That wouldn't be nice in a function declaration. What I have in mind now is changing them all to int, and cast to x86_reg (or use local x86_reg variables) only inside x86 or mmx ifdefs. What do you think about this approach?
also these unused vars could need a av_unused
The ones in the function declaration? Does it make any difference?
On Thu, Jan 14, 2010 at 04:25:01AM -0200, Ramiro Polla wrote:
On Mon, Jan 11, 2010 at 12:51 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Mon, Jan 11, 2010 at 05:21:19AM -0200, Ramiro Polla wrote: [...]
?swscale.c | ? ?6 +++--- ?1 file changed, 3 insertions(+), 3 deletions(-) 104e7c17caec26638ce07bb04dc15eaa0d9133fb ?0003-int-long-for-width-parameter-in-rgb48-to-YV12-fun.patch From da0f8f381054f23a32251981354ec56fb1232a56 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla at gmail.com> Date: Mon, 11 Jan 2010 05:04:09 -0200 Subject: [PATCH] int -> long for width parameter in rgb48 to YV12 functions.
dont we have a proper type for that?
You mean x86_reg? That wouldn't be nice in a function declaration.
What I have in mind now is changing them all to int, and cast to x86_reg (or use local x86_reg variables) only inside x86 or mmx ifdefs. What do you think about this approach?
whats the problem with x86_reg in a declaration? maybe we need an array_index type and should use this throughout ffmpeg
also these unused vars could need a av_unused
The ones in the function declaration? Does it make any difference?
lack of it might lead to compiler warnings [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No snowflake in an avalanche ever feels responsible. -- Voltaire
Michael Niedermayer <michaelni at gmx.at> writes:
On Thu, Jan 14, 2010 at 04:25:01AM -0200, Ramiro Polla wrote:
On Mon, Jan 11, 2010 at 12:51 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Mon, Jan 11, 2010 at 05:21:19AM -0200, Ramiro Polla wrote: [...]
?swscale.c | ? ?6 +++--- ?1 file changed, 3 insertions(+), 3 deletions(-) 104e7c17caec26638ce07bb04dc15eaa0d9133fb ?0003-int-long-for-width-parameter-in-rgb48-to-YV12-fun.patch From da0f8f381054f23a32251981354ec56fb1232a56 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla at gmail.com> Date: Mon, 11 Jan 2010 05:04:09 -0200 Subject: [PATCH] int -> long for width parameter in rgb48 to YV12 functions.
dont we have a proper type for that?
You mean x86_reg? That wouldn't be nice in a function declaration.
What I have in mind now is changing them all to int, and cast to x86_reg (or use local x86_reg variables) only inside x86 or mmx ifdefs. What do you think about this approach?
whats the problem with x86_reg in a declaration?
Using x86_reg in non-x86-specific code is ugly. In fact, it shouldn't even compile. -- M?ns Rullg?rd mans at mansr.com
2010/1/14 M?ns Rullg?rd <mans at mansr.com>:
Michael Niedermayer <michaelni at gmx.at> writes:
On Thu, Jan 14, 2010 at 04:25:01AM -0200, Ramiro Polla wrote:
On Mon, Jan 11, 2010 at 12:51 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Mon, Jan 11, 2010 at 05:21:19AM -0200, Ramiro Polla wrote: [...]
?swscale.c | ? ?6 +++--- ?1 file changed, 3 insertions(+), 3 deletions(-) 104e7c17caec26638ce07bb04dc15eaa0d9133fb ?0003-int-long-for-width-parameter-in-rgb48-to-YV12-fun.patch From da0f8f381054f23a32251981354ec56fb1232a56 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla at gmail.com> Date: Mon, 11 Jan 2010 05:04:09 -0200 Subject: [PATCH] int -> long for width parameter in rgb48 to YV12 functions.
dont we have a proper type for that?
You mean x86_reg? That wouldn't be nice in a function declaration.
What I have in mind now is changing them all to int, and cast to x86_reg (or use local x86_reg variables) only inside x86 or mmx ifdefs. What do you think about this approach?
whats the problem with x86_reg in a declaration?
Using x86_reg in non-x86-specific code is ugly. ?In fact, it shouldn't even compile.
in libavutil:x86_cpu.h #if ARCH_X86_64 [...] typedef int64_t x86_reg; [...] #elif ARCH_X86_32 [...] typedef int32_t x86_reg; [...] #else typedef int x86_reg; #endif So it does compile for now, but I'd like to remove that #else as well. What do you think about that array_index type michael mentioned?
Ramiro Polla <ramiro.polla at gmail.com> writes:
2010/1/14 M?ns Rullg?rd <mans at mansr.com>:
Michael Niedermayer <michaelni at gmx.at> writes:
On Thu, Jan 14, 2010 at 04:25:01AM -0200, Ramiro Polla wrote:
On Mon, Jan 11, 2010 at 12:51 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Mon, Jan 11, 2010 at 05:21:19AM -0200, Ramiro Polla wrote: [...]
?swscale.c | ? ?6 +++--- ?1 file changed, 3 insertions(+), 3 deletions(-) 104e7c17caec26638ce07bb04dc15eaa0d9133fb ?0003-int-long-for-width-parameter-in-rgb48-to-YV12-fun.patch From da0f8f381054f23a32251981354ec56fb1232a56 Mon Sep 17 00:00:00 2001 From: Ramiro Polla <ramiro.polla at gmail.com> Date: Mon, 11 Jan 2010 05:04:09 -0200 Subject: [PATCH] int -> long for width parameter in rgb48 to YV12 functions.
dont we have a proper type for that?
You mean x86_reg? That wouldn't be nice in a function declaration.
What I have in mind now is changing them all to int, and cast to x86_reg (or use local x86_reg variables) only inside x86 or mmx ifdefs. What do you think about this approach?
whats the problem with x86_reg in a declaration?
Using x86_reg in non-x86-specific code is ugly. ?In fact, it shouldn't even compile.
in libavutil:x86_cpu.h #if ARCH_X86_64 [...] typedef int64_t x86_reg; [...] #elif ARCH_X86_32 [...] typedef int32_t x86_reg; [...] #else typedef int x86_reg; #endif
So it does compile for now, but I'd like to remove that #else as well.
IMO x86_cpu.h should not be included outside x86-specific code.
What do you think about that array_index type michael mentioned?
What is special about array indexes? I thought the problem was ensuring the C type matches the operand type used in inline asm. A more generic type name always defined to the native word size of the machine might be a better option. -- M?ns Rullg?rd mans at mansr.com
On Thu, Jan 14, 2010 at 02:10:56PM +0000, M?ns Rullg?rd wrote:
Ramiro Polla <ramiro.polla at gmail.com> writes:
2010/1/14 M?ns Rullg?rd <mans at mansr.com>:
Michael Niedermayer <michaelni at gmx.at> writes:
On Thu, Jan 14, 2010 at 04:25:01AM -0200, Ramiro Polla wrote:
On Mon, Jan 11, 2010 at 12:51 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Mon, Jan 11, 2010 at 05:21:19AM -0200, Ramiro Polla wrote: [...] > ?swscale.c | ? ?6 +++--- > ?1 file changed, 3 insertions(+), 3 deletions(-) > 104e7c17caec26638ce07bb04dc15eaa0d9133fb ?0003-int-long-for-width-parameter-in-rgb48-to-YV12-fun.patch > From da0f8f381054f23a32251981354ec56fb1232a56 Mon Sep 17 00:00:00 2001 > From: Ramiro Polla <ramiro.polla at gmail.com> > Date: Mon, 11 Jan 2010 05:04:09 -0200 > Subject: [PATCH] int -> long for width parameter in rgb48 to YV12 functions.
dont we have a proper type for that?
You mean x86_reg? That wouldn't be nice in a function declaration.
What I have in mind now is changing them all to int, and cast to x86_reg (or use local x86_reg variables) only inside x86 or mmx ifdefs. What do you think about this approach?
whats the problem with x86_reg in a declaration?
Using x86_reg in non-x86-specific code is ugly. ?In fact, it shouldn't even compile.
in libavutil:x86_cpu.h #if ARCH_X86_64 [...] typedef int64_t x86_reg; [...] #elif ARCH_X86_32 [...] typedef int32_t x86_reg; [...] #else typedef int x86_reg; #endif
So it does compile for now, but I'd like to remove that #else as well.
IMO x86_cpu.h should not be included outside x86-specific code.
What do you think about that array_index type michael mentioned?
What is special about array indexes? I thought the problem was ensuring the C type matches the operand type used in inline asm.
64bit array indexes will likely be faster on x86_64 in C code (this of course needs a test, a codec like h264 seems like the most important place to try it. If its not faster than we can drop the array_index idea)
A more generic type name always defined to the native word size of the machine might be a better option.
Whats faster for array indexes might not be faster for multiplications also it would totally kill any compiler based SIMD generation as variables suddenly would be bigger then needed and whats native on a cpu supporting multiple sizes ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Many that live deserve death. And some that die deserve life. Can you give it to them? Then do not be too eager to deal out death in judgement. For even the very wise cannot see all ends. -- Gandalf
Michael Niedermayer <michaelni at gmx.at> writes:
On Thu, Jan 14, 2010 at 02:10:56PM +0000, M?ns Rullg?rd wrote:
Ramiro Polla <ramiro.polla at gmail.com> writes:
2010/1/14 M?ns Rullg?rd <mans at mansr.com>:
Michael Niedermayer <michaelni at gmx.at> writes:
On Thu, Jan 14, 2010 at 04:25:01AM -0200, Ramiro Polla wrote:
On Mon, Jan 11, 2010 at 12:51 PM, Michael Niedermayer <michaelni at gmx.at> wrote: > On Mon, Jan 11, 2010 at 05:21:19AM -0200, Ramiro Polla wrote: [...] >> ?swscale.c | ? ?6 +++--- >> ?1 file changed, 3 insertions(+), 3 deletions(-) >> 104e7c17caec26638ce07bb04dc15eaa0d9133fb ?0003-int-long-for-width-parameter-in-rgb48-to-YV12-fun.patch >> From da0f8f381054f23a32251981354ec56fb1232a56 Mon Sep 17 00:00:00 2001 >> From: Ramiro Polla <ramiro.polla at gmail.com> >> Date: Mon, 11 Jan 2010 05:04:09 -0200 >> Subject: [PATCH] int -> long for width parameter in rgb48 to YV12 functions. > > dont we have a proper type for that?
You mean x86_reg? That wouldn't be nice in a function declaration.
What I have in mind now is changing them all to int, and cast to x86_reg (or use local x86_reg variables) only inside x86 or mmx ifdefs. What do you think about this approach?
whats the problem with x86_reg in a declaration?
Using x86_reg in non-x86-specific code is ugly. ?In fact, it shouldn't even compile.
in libavutil:x86_cpu.h #if ARCH_X86_64 [...] typedef int64_t x86_reg; [...] #elif ARCH_X86_32 [...] typedef int32_t x86_reg; [...] #else typedef int x86_reg; #endif
So it does compile for now, but I'd like to remove that #else as well.
IMO x86_cpu.h should not be included outside x86-specific code.
What do you think about that array_index type michael mentioned?
What is special about array indexes? I thought the problem was ensuring the C type matches the operand type used in inline asm.
64bit array indexes will likely be faster on x86_64 in C code (this of course needs a test, a codec like h264 seems like the most important place to try it. If its not faster than we can drop the array_index idea)
Have I mentioned that I hate x86? -- M?ns Rullg?rd mans at mansr.com
On Thu, Jan 14, 2010 at 03:20:07PM +0000, M?ns Rullg?rd wrote:
Michael Niedermayer <michaelni at gmx.at> writes:
On Thu, Jan 14, 2010 at 02:10:56PM +0000, M?ns Rullg?rd wrote:
Ramiro Polla <ramiro.polla at gmail.com> writes:
2010/1/14 M?ns Rullg?rd <mans at mansr.com>:
Michael Niedermayer <michaelni at gmx.at> writes:
On Thu, Jan 14, 2010 at 04:25:01AM -0200, Ramiro Polla wrote: > On Mon, Jan 11, 2010 at 12:51 PM, Michael Niedermayer <michaelni at gmx.at> wrote: > > On Mon, Jan 11, 2010 at 05:21:19AM -0200, Ramiro Polla wrote: [...] > >> ?swscale.c | ? ?6 +++--- > >> ?1 file changed, 3 insertions(+), 3 deletions(-) > >> 104e7c17caec26638ce07bb04dc15eaa0d9133fb ?0003-int-long-for-width-parameter-in-rgb48-to-YV12-fun.patch > >> From da0f8f381054f23a32251981354ec56fb1232a56 Mon Sep 17 00:00:00 2001 > >> From: Ramiro Polla <ramiro.polla at gmail.com> > >> Date: Mon, 11 Jan 2010 05:04:09 -0200 > >> Subject: [PATCH] int -> long for width parameter in rgb48 to YV12 functions. > > > > dont we have a proper type for that? > > You mean x86_reg? That wouldn't be nice in a function declaration. > > What I have in mind now is changing them all to int, and cast to > x86_reg (or use local x86_reg variables) only inside x86 or mmx > ifdefs. What do you think about this approach?
whats the problem with x86_reg in a declaration?
Using x86_reg in non-x86-specific code is ugly. ?In fact, it shouldn't even compile.
in libavutil:x86_cpu.h #if ARCH_X86_64 [...] typedef int64_t x86_reg; [...] #elif ARCH_X86_32 [...] typedef int32_t x86_reg; [...] #else typedef int x86_reg; #endif
So it does compile for now, but I'd like to remove that #else as well.
IMO x86_cpu.h should not be included outside x86-specific code.
What do you think about that array_index type michael mentioned?
What is special about array indexes? I thought the problem was ensuring the C type matches the operand type used in inline asm.
64bit array indexes will likely be faster on x86_64 in C code (this of course needs a test, a codec like h264 seems like the most important place to try it. If its not faster than we can drop the array_index idea)
Have I mentioned that I hate x86?
yes [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I hate to see young programmers poisoned by the kind of thinking Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
On Thu, Jan 14, 2010 at 1:11 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Thu, Jan 14, 2010 at 02:10:56PM +0000, M?ns Rullg?rd wrote:
Ramiro Polla <ramiro.polla at gmail.com> writes:
2010/1/14 M?ns Rullg?rd <mans at mansr.com>:
Michael Niedermayer <michaelni at gmx.at> writes:
On Thu, Jan 14, 2010 at 04:25:01AM -0200, Ramiro Polla wrote:
On Mon, Jan 11, 2010 at 12:51 PM, Michael Niedermayer <michaelni at gmx.at> wrote: > On Mon, Jan 11, 2010 at 05:21:19AM -0200, Ramiro Polla wrote: [...] >> ?swscale.c | ? ?6 +++--- >> ?1 file changed, 3 insertions(+), 3 deletions(-) >> 104e7c17caec26638ce07bb04dc15eaa0d9133fb ?0003-int-long-for-width-parameter-in-rgb48-to-YV12-fun.patch >> From da0f8f381054f23a32251981354ec56fb1232a56 Mon Sep 17 00:00:00 2001 >> From: Ramiro Polla <ramiro.polla at gmail.com> >> Date: Mon, 11 Jan 2010 05:04:09 -0200 >> Subject: [PATCH] int -> long for width parameter in rgb48 to YV12 functions. > > dont we have a proper type for that?
You mean x86_reg? That wouldn't be nice in a function declaration.
What I have in mind now is changing them all to int, and cast to x86_reg (or use local x86_reg variables) only inside x86 or mmx ifdefs. What do you think about this approach?
whats the problem with x86_reg in a declaration?
Using x86_reg in non-x86-specific code is ugly. ?In fact, it shouldn't even compile.
in libavutil:x86_cpu.h #if ARCH_X86_64 [...] typedef int64_t x86_reg; [...] #elif ARCH_X86_32 [...] typedef int32_t x86_reg; [...] #else typedef int x86_reg; #endif
So it does compile for now, but I'd like to remove that #else as well.
IMO x86_cpu.h should not be included outside x86-specific code.
What do you think about that array_index type michael mentioned?
What is special about array indexes? ?I thought the problem was ensuring the C type matches the operand type used in inline asm.
64bit array indexes will likely be faster on x86_64 in C code (this of course needs a test, a codec like h264 seems like the most ?important place to try it. If its not faster than we can drop the ?array_index idea)
So basically we would still have some sort of type that is the width of the cpu's register and use it in the declaration? Attached patch is just an example (it's a hack) that uses x86_reg as cpu_reg so the code isn't x86-centric. I don't really understand how that will help here. The width is int, so at some point it will have to be changed to cpu_reg, be it before calling, or in the function prologue. From the asm it looks like it just shuffles one movslq from caller to callee.
On Sun, Jan 24, 2010 at 03:14:47AM -0200, Ramiro Polla wrote:
On Thu, Jan 14, 2010 at 1:11 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Thu, Jan 14, 2010 at 02:10:56PM +0000, M?ns Rullg?rd wrote:
Ramiro Polla <ramiro.polla at gmail.com> writes:
2010/1/14 M?ns Rullg?rd <mans at mansr.com>:
Michael Niedermayer <michaelni at gmx.at> writes:
On Thu, Jan 14, 2010 at 04:25:01AM -0200, Ramiro Polla wrote: > On Mon, Jan 11, 2010 at 12:51 PM, Michael Niedermayer <michaelni at gmx.at> wrote: > > On Mon, Jan 11, 2010 at 05:21:19AM -0200, Ramiro Polla wrote: [...] > >> ?swscale.c | ? ?6 +++--- > >> ?1 file changed, 3 insertions(+), 3 deletions(-) > >> 104e7c17caec26638ce07bb04dc15eaa0d9133fb ?0003-int-long-for-width-parameter-in-rgb48-to-YV12-fun.patch > >> From da0f8f381054f23a32251981354ec56fb1232a56 Mon Sep 17 00:00:00 2001 > >> From: Ramiro Polla <ramiro.polla at gmail.com> > >> Date: Mon, 11 Jan 2010 05:04:09 -0200 > >> Subject: [PATCH] int -> long for width parameter in rgb48 to YV12 functions. > > > > dont we have a proper type for that? > > You mean x86_reg? That wouldn't be nice in a function declaration. > > What I have in mind now is changing them all to int, and cast to > x86_reg (or use local x86_reg variables) only inside x86 or mmx > ifdefs. What do you think about this approach?
whats the problem with x86_reg in a declaration?
Using x86_reg in non-x86-specific code is ugly. ?In fact, it shouldn't even compile.
in libavutil:x86_cpu.h #if ARCH_X86_64 [...] typedef int64_t x86_reg; [...] #elif ARCH_X86_32 [...] typedef int32_t x86_reg; [...] #else typedef int x86_reg; #endif
So it does compile for now, but I'd like to remove that #else as well.
IMO x86_cpu.h should not be included outside x86-specific code.
What do you think about that array_index type michael mentioned?
What is special about array indexes? ?I thought the problem was ensuring the C type matches the operand type used in inline asm.
64bit array indexes will likely be faster on x86_64 in C code (this of course needs a test, a codec like h264 seems like the most ?important place to try it. If its not faster than we can drop the ?array_index idea)
So basically we would still have some sort of type that is the width of the cpu's register and use it in the declaration? Attached patch is just an example (it's a hack) that uses x86_reg as cpu_reg so the code isn't x86-centric.
I don't really understand how that will help here. The width is int, so at some point it will have to be changed to cpu_reg, be it before calling, or in the function prologue. From the asm it looks like it just shuffles one movslq from caller to callee.
the gain happens when you change the variables used to calculate the index also to it. You could also try to make the index unsigned but make sure it cant be negative if you try this [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Awnsering whenever a program halts or runs forever is On a turing machine, in general impossible (turings halting problem). On any real computer, always possible as a real computer has a finite number of states N, and will either halt in less than N cycles or never halt.
Hello Michael, On Sun, Jan 24, 2010 at 8:31 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
the gain happens when you change the variables used to calculate the index also to it. You could also try to make the index unsigned but make sure it cant be negative if you try this
Sorry but I still don't understand how that will be of use here in libswscale. I've tried forcing int32_t and int64_t for x86_64 in some of those functions (some xxxTo(Y|UV), hScale and the fast bilinear ones), in all C, MMX and MMX2. All I can see is the expansion from 32-bit to 64-bit being changed from caller and callee. There is no difference in the inner loop, nor in how gcc addresses the the src and dst arrays. I know you're busy with a bunch of stuff, but could you please describe what you're thinking of a little better (for example with the actual functions in libswscale where using the register width in the function declaration is much better, and if possible the diff from the asm they generate). It seems logical that width is int (as it's declared in the context), and not necessarily long, so I've attached a patch that changes it from long to int in xxxTo(Y|UV), hScale and fast bilinear. Ramiro Polla
On Tue, Feb 02, 2010 at 04:10:06PM -0200, Ramiro Polla wrote:
Hello Michael,
On Sun, Jan 24, 2010 at 8:31 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
the gain happens when you change the variables used to calculate the index also to it. You could also try to make the index unsigned but make sure it cant be negative if you try this
Sorry but I still don't understand how that will be of use here in libswscale. I've tried forcing int32_t and int64_t for x86_64 in some of those functions (some xxxTo(Y|UV), hScale and the fast bilinear ones), in all C, MMX and MMX2. All I can see is the expansion from 32-bit to 64-bit being changed from caller and callee. There is no difference in the inner loop, nor in how gcc addresses the the src and dst arrays.
maybe theres no gain for swscale, i cant say without looking at the asm gcc generates. i know that in h264 gcc filled some functions with 32->64 sign extension code in the inner loops. if there are no sign extension instructions betweem registers in the inner loop then theres no problem you could fix with this [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
On Tue, Feb 02, 2010 at 08:01:26PM +0100, Michael Niedermayer wrote:
On Tue, Feb 02, 2010 at 04:10:06PM -0200, Ramiro Polla wrote:
Hello Michael,
On Sun, Jan 24, 2010 at 8:31 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
the gain happens when you change the variables used to calculate the index also to it. You could also try to make the index unsigned but make sure it cant be negative if you try this
Sorry but I still don't understand how that will be of use here in libswscale. I've tried forcing int32_t and int64_t for x86_64 in some of those functions (some xxxTo(Y|UV), hScale and the fast bilinear ones), in all C, MMX and MMX2. All I can see is the expansion from 32-bit to 64-bit being changed from caller and callee. There is no difference in the inner loop, nor in how gcc addresses the the src and dst arrays.
maybe theres no gain for swscale, i cant say without looking at the asm gcc generates. i know that in h264 gcc filled some functions with 32->64 sign extension code in the inner loops.
Which compilation options have you been using? IIRC sign extension is only an issue when gcc uses the indexed load instructions, which it does not use at all anymore when optimizing for a more modern CPU (which means it ends up being even shorter on registers and thus creating larger and slower code with probably theoretically better instruction scheduling).
On Tue, Feb 02, 2010 at 08:21:15PM +0100, Reimar D?ffinger wrote:
On Tue, Feb 02, 2010 at 08:01:26PM +0100, Michael Niedermayer wrote:
On Tue, Feb 02, 2010 at 04:10:06PM -0200, Ramiro Polla wrote:
Hello Michael,
On Sun, Jan 24, 2010 at 8:31 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
the gain happens when you change the variables used to calculate the index also to it. You could also try to make the index unsigned but make sure it cant be negative if you try this
Sorry but I still don't understand how that will be of use here in libswscale. I've tried forcing int32_t and int64_t for x86_64 in some of those functions (some xxxTo(Y|UV), hScale and the fast bilinear ones), in all C, MMX and MMX2. All I can see is the expansion from 32-bit to 64-bit being changed from caller and callee. There is no difference in the inner loop, nor in how gcc addresses the the src and dst arrays.
maybe theres no gain for swscale, i cant say without looking at the asm gcc generates. i know that in h264 gcc filled some functions with 32->64 sign extension code in the inner loops.
Which compilation options have you been using?
default of ffmpeg & gcc-4.4 also a quick grep movslq libavcodec/h264_loopfilter.S | grep -v '(' 68 136 1220 and with -mtune=core2 -march=core2 -mcpu=core2 grep movslq libavcodec/h264_loopfilter.S | grep -v '(' |wc 68 136 1226 so no, its not helping it still does produce all the register-register sign extensions
IIRC sign extension is only an issue when gcc uses the indexed load instructions, which it does not use at all anymore when optimizing for a more modern CPU (which means it ends up being even shorter on registers and thus creating larger and slower code with probably theoretically better instruction scheduling). _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel at mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
-- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I am the wisest man alive, for I know one thing, and that is that I know nothing. -- Socrates
Hi, On Tue, Feb 2, 2010 at 5:42 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, Feb 02, 2010 at 08:21:15PM +0100, Reimar D?ffinger wrote:
On Tue, Feb 02, 2010 at 08:01:26PM +0100, Michael Niedermayer wrote:
On Tue, Feb 02, 2010 at 04:10:06PM -0200, Ramiro Polla wrote:
Hello Michael,
On Sun, Jan 24, 2010 at 8:31 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
the gain happens when you change the variables used to calculate the index also to it. You could also try to make the index unsigned but make sure it cant be negative if you try this
Sorry but I still don't understand how that will be of use here in libswscale. I've tried forcing int32_t and int64_t for x86_64 in some of those functions (some xxxTo(Y|UV), hScale and the fast bilinear ones), in all C, MMX and MMX2. All I can see is the expansion from 32-bit to 64-bit being changed from caller and callee. There is no difference in the inner loop, nor in how gcc addresses the the src and dst arrays.
maybe theres no gain for swscale, i cant say without looking at the asm gcc generates. i know that in h264 gcc filled some functions with 32->64 sign extension code in the inner loops.
Which compilation options have you been using?
default of ffmpeg & gcc-4.4 also a quick grep movslq libavcodec/h264_loopfilter.S | grep -v '(' ? ?68 ? ? 136 ? ?1220
and with -mtune=core2 -march=core2 -mcpu=core2 grep movslq libavcodec/h264_loopfilter.S | grep -v '(' |wc ? ?68 ? ? 136 ? ?1226
so no, its not helping it still does produce all the register-register sign extensions
Hmm, I think I understand now what you mean... This is what the asm of some functions look like when things get changes from long to int. I'll put the sizes of some functions as in <name> <size with long> <size with int> <int - long>, along with their differences (mostly only prologues). All tested with gcc 4.4.1 from ubuntu 9.10: nv12ToUV_MMX 77 87 10 BEToUV_MMX 84 90 6 and similar _MMX functions. int lea (%r8,%r8,1),%r9d movslq %r8d,%rax neg %r8d movslq %r8d,%r8 add %rax,%rdi add %rax,%rsi movslq %r9d,%r9 add %r9,%rdx add %r9,%rcx movq 0x0,%mm4 long: lea (%r8,%r8,1),%rax mov %r8,%r9 add %r8,%rdi neg %r9 add %r8,%rsi add %rax,%rdx add %rax,%rcx movq 0x0,%mm4 bgr24ToUV_half_3DNow 142 172 30 int: test %r8d,%r8d push %rbx jle 1cd8a <bgr24ToUV_half_3DNow+0xaa> sub $0x1,%r8d xor %eax,%eax lea 0x3(%r8,%r8,2),%rbx add %rbx,%rbx movzbl (%rdx,%rax,1),%ecx movzbl 0x3(%rdx,%rax,1),%r9d movzbl 0x4(%rdx,%rax,1),%r10d movzbl 0x5(%rdx,%rax,1),%r8d long: test %r8,%r8 jle 1cb1c <bgr24ToUV_half_3DNow+0x8c> lea (%rdi,%r8,1),%r8 movzbl (%rdx),%eax movzbl 0x3(%rdx),%r9d movzbl 0x4(%rdx),%r10d movzbl 0x5(%rdx),%ecx rgb32ToUV 139 143 4 int sub $0x1,%r8d lea 0x4(%rdx,%r8,4),%r11 mov (%rdx),%ecx long push %rbx shl $0x2,%r8 xor %ecx,%ecx mov (%rdx,%rcx,1),%r9d all hyscale_fast functions have only one more movslq in the int version. Then many have this difference where the int version uses sub and lea while the long version uses either add %reg,%reg or shl $2, %reg. abgrToA 37 45 8 BEToUV_C 51 51 0 nv12ToUV_C 48 48 0 int sub $0x1,%r8d lea 0x2(%r8,%r8,1),%r8 long add %r8,%r8 rgb15ToUV 141 143 2 long uses rbx (as in it pushes and pops rbx) while the int version doesn't, long accesses arrays with movzwl (%rdx,%rax,1),%r9d instead of movzwl (%rdx),%ecx in the inner loop (I don't know what difference this makes). long uses add %r8,%r8 instead of sub & lea. Then there's: rgb15ToUV_half 166 174 8 int sub $0x1,%r8d lea 0x4(%rdx,%r8,4),%r10 long lea (%rdx,%r8,4),%r10 Very few functions are larger with long such as: rgb15ToY 95 84 -11 int uses sub & lea instead of add. long uses more 64-bit registers so the instructions are larger. And on to the caller, swScale_C 10319 10082 -237 long has 9 more movslq, uses more stack I haven't checked all functions though. The final size (with runtime cpudetect): 841336 swscale_ints.o 841024 swscale_longs.o The number of movslq between registers: $ objdump -d swscale_ints.o | grep movslq | grep -v "(" | wc -l 1038 $ objdump -d swscale_longs.o | grep movslq | grep -v "(" | wc -l 927 No speed differences were ever noticed. Dark_Shikari tells me a movslq between registers is 1uop... As for other architectures, the arm and ppc I have would have made no difference since they're not 64-bit. I've attached a patch which adds an array_index type, if that's what you had in mind. Otherwise I really don't know what to do. Long is being misused here, and breaks compilation on mingw-w64.
On Tue, Feb 02, 2010 at 11:30:24PM -0200, Ramiro Polla wrote:
Hi,
On Tue, Feb 2, 2010 at 5:42 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, Feb 02, 2010 at 08:21:15PM +0100, Reimar D?ffinger wrote:
On Tue, Feb 02, 2010 at 08:01:26PM +0100, Michael Niedermayer wrote:
On Tue, Feb 02, 2010 at 04:10:06PM -0200, Ramiro Polla wrote:
Hello Michael,
On Sun, Jan 24, 2010 at 8:31 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
the gain happens when you change the variables used to calculate the index also to it. You could also try to make the index unsigned but make sure it cant be negative if you try this
Sorry but I still don't understand how that will be of use here in libswscale. I've tried forcing int32_t and int64_t for x86_64 in some of those functions (some xxxTo(Y|UV), hScale and the fast bilinear ones), in all C, MMX and MMX2. All I can see is the expansion from 32-bit to 64-bit being changed from caller and callee. There is no difference in the inner loop, nor in how gcc addresses the the src and dst arrays.
maybe theres no gain for swscale, i cant say without looking at the asm gcc generates. i know that in h264 gcc filled some functions with 32->64 sign extension code in the inner loops.
Which compilation options have you been using?
default of ffmpeg & gcc-4.4 also a quick grep movslq libavcodec/h264_loopfilter.S | grep -v '(' ? ?68 ? ? 136 ? ?1220
and with -mtune=core2 -march=core2 -mcpu=core2 grep movslq libavcodec/h264_loopfilter.S | grep -v '(' |wc ? ?68 ? ? 136 ? ?1226
so no, its not helping it still does produce all the register-register sign extensions
Hmm, I think I understand now what you mean... This is what the asm of some functions look like when things get changes from long to int. I'll put the sizes of some functions as in <name> <size with long> <size with int> <int - long>, along with their differences (mostly only prologues). All tested with gcc 4.4.1 from ubuntu 9.10:
nv12ToUV_MMX 77 87 10 BEToUV_MMX 84 90 6 and similar _MMX functions. int lea (%r8,%r8,1),%r9d movslq %r8d,%rax neg %r8d movslq %r8d,%r8 add %rax,%rdi add %rax,%rsi movslq %r9d,%r9 add %r9,%rdx add %r9,%rcx movq 0x0,%mm4 long: lea (%r8,%r8,1),%rax mov %r8,%r9 add %r8,%rdi neg %r9 add %r8,%rsi add %rax,%rdx add %rax,%rcx movq 0x0,%mm4
bgr24ToUV_half_3DNow 142 172 30 int: test %r8d,%r8d push %rbx jle 1cd8a <bgr24ToUV_half_3DNow+0xaa> sub $0x1,%r8d xor %eax,%eax lea 0x3(%r8,%r8,2),%rbx add %rbx,%rbx movzbl (%rdx,%rax,1),%ecx movzbl 0x3(%rdx,%rax,1),%r9d movzbl 0x4(%rdx,%rax,1),%r10d movzbl 0x5(%rdx,%rax,1),%r8d long: test %r8,%r8 jle 1cb1c <bgr24ToUV_half_3DNow+0x8c> lea (%rdi,%r8,1),%r8 movzbl (%rdx),%eax movzbl 0x3(%rdx),%r9d movzbl 0x4(%rdx),%r10d movzbl 0x5(%rdx),%ecx
rgb32ToUV 139 143 4 int sub $0x1,%r8d lea 0x4(%rdx,%r8,4),%r11 mov (%rdx),%ecx long push %rbx shl $0x2,%r8 xor %ecx,%ecx mov (%rdx,%rcx,1),%r9d
all hyscale_fast functions have only one more movslq in the int version.
Then many have this difference where the int version uses sub and lea while the long version uses either add %reg,%reg or shl $2, %reg.
abgrToA 37 45 8 BEToUV_C 51 51 0 nv12ToUV_C 48 48 0 int sub $0x1,%r8d lea 0x2(%r8,%r8,1),%r8 long add %r8,%r8
rgb15ToUV 141 143 2 long uses rbx (as in it pushes and pops rbx) while the int version doesn't, long accesses arrays with movzwl (%rdx,%rax,1),%r9d instead of movzwl (%rdx),%ecx in the inner loop (I don't know what difference this makes). long uses add %r8,%r8 instead of sub & lea.
Then there's: rgb15ToUV_half 166 174 8 int sub $0x1,%r8d lea 0x4(%rdx,%r8,4),%r10 long lea (%rdx,%r8,4),%r10
Very few functions are larger with long such as: rgb15ToY 95 84 -11 int uses sub & lea instead of add. long uses more 64-bit registers so the instructions are larger.
And on to the caller,
swScale_C 10319 10082 -237 long has 9 more movslq, uses more stack
I haven't checked all functions though.
The final size (with runtime cpudetect): 841336 swscale_ints.o 841024 swscale_longs.o
The number of movslq between registers: $ objdump -d swscale_ints.o | grep movslq | grep -v "(" | wc -l 1038 $ objdump -d swscale_longs.o | grep movslq | grep -v "(" | wc -l 927
No speed differences were ever noticed. Dark_Shikari tells me a movslq between registers is 1uop...
As for other architectures, the arm and ppc I have would have made no difference since they're not 64-bit.
I've attached a patch which adds an array_index type, if that's what you had in mind.
Otherwise I really don't know what to do. Long is being misused here, and breaks compilation on mingw-w64.
if theres no speed difference i dont mind int being used I also dont mind the array_index stuff being added to libavutil [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Many that live deserve death. And some that die deserve life. Can you give it to them? Then do not be too eager to deal out death in judgement. For even the very wise cannot see all ends. -- Gandalf
On Tue, Feb 2, 2010 at 11:48 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
if theres no speed difference i dont mind int being used
Patch attached. It changes all uses of long meant for width, height and stride to int. Besides renaming, it adds these chunks to hcscale_fast(): @@ -2469,6 +2469,7 @@ static inline void RENAME(hcscale_fast)(SwsContext *c, int16_t *dst, } } else { #endif /* COMPILE_TEMPLATE_MMX2 */ + x86_reg dstWidth_reg = dstWidth; x86_reg xInc_shr16 = (x86_reg) (xInc >> 16); uint16_t xInc_mask = xInc & 0xffff; __asm__ volatile( @@ -2497,9 +2498,9 @@ static inline void RENAME(hcscale_fast)(SwsContext *c, int16_t *dst, /* GCC 3.3 makes MPlayer crash on IA-32 machines when using "g" operand here, which is needed to support GCC 4.0. */ #if ARCH_X86_64 && AV_GCC_VERSION_AT_LEAST(3,4) - :: "m" (src1), "m" (dst), "g" (dstWidth), "m" (xInc_shr16), "m" (xInc_mask), + :: "m" (src1), "m" (dst), "g" (dstWidth_reg), "m" (xInc_shr16), "m" (xInc_mask), #else - :: "m" (src1), "m" (dst), "m" (dstWidth), "m" (xInc_shr16), "m" (xInc_mask), + :: "m" (src1), "m" (dst), "m" (dstWidth_reg), "m" (xInc_shr16), "m" (xInc_mask), #endif "r" (src2) : "%"REG_a, "%"REG_d, "%ecx", "%"REG_D, "%esi" Tested on linux x86, x86_64, mingw32 x86, mingw-w64 x86_64, and ppc. All both with and without gpl. Ramiro Polla
On Wed, Feb 24, 2010 at 12:49 AM, Ramiro Polla <ramiro.polla at gmail.com> wrote:
On Tue, Feb 2, 2010 at 11:48 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
if theres no speed difference i dont mind int being used
Patch attached. It changes all uses of long meant for width, height and stride to int. Besides renaming, it adds these chunks to hcscale_fast(): @@ -2469,6 +2469,7 @@ static inline void RENAME(hcscale_fast)(SwsContext *c, int16_t *dst, ? ? ? ? } ? ? } else { ?#endif /* COMPILE_TEMPLATE_MMX2 */ + ? ? ? ?x86_reg dstWidth_reg = dstWidth; ? ? ? ? x86_reg xInc_shr16 = (x86_reg) (xInc >> 16); ? ? ? ? uint16_t xInc_mask = xInc & 0xffff; ? ? ? ? __asm__ volatile( @@ -2497,9 +2498,9 @@ static inline void RENAME(hcscale_fast)(SwsContext *c, int16_t *dst, ?/* GCC 3.3 makes MPlayer crash on IA-32 machines when using "g" operand here, ?which is needed to support GCC 4.0. */ ?#if ARCH_X86_64 && AV_GCC_VERSION_AT_LEAST(3,4) - ? ? ? ? ? ?:: "m" (src1), "m" (dst), "g" (dstWidth), "m" (xInc_shr16), "m" (xInc_mask), + ? ? ? ? ? ?:: "m" (src1), "m" (dst), "g" (dstWidth_reg), "m" (xInc_shr16), "m" (xInc_mask), ?#else - ? ? ? ? ? ?:: "m" (src1), "m" (dst), "m" (dstWidth), "m" (xInc_shr16), "m" (xInc_mask), + ? ? ? ? ? ?:: "m" (src1), "m" (dst), "m" (dstWidth_reg), "m" (xInc_shr16), "m" (xInc_mask), ?#endif ? ? ? ? ? ? "r" (src2) ? ? ? ? ? ? : "%"REG_a, "%"REG_d, "%ecx", "%"REG_D, "%esi"
Tested on linux x86, x86_64, mingw32 x86, mingw-w64 x86_64, and ppc. All both with and without gpl.
Ping.
On Wed, Feb 24, 2010 at 12:49:13AM -0300, Ramiro Polla wrote:
On Tue, Feb 2, 2010 at 11:48 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
if theres no speed difference i dont mind int being used
Patch attached. It changes all uses of long meant for width, height and stride to int. Besides renaming, it adds these chunks to hcscale_fast(): @@ -2469,6 +2469,7 @@ static inline void RENAME(hcscale_fast)(SwsContext *c, int16_t *dst, } } else { #endif /* COMPILE_TEMPLATE_MMX2 */ + x86_reg dstWidth_reg = dstWidth; x86_reg xInc_shr16 = (x86_reg) (xInc >> 16); uint16_t xInc_mask = xInc & 0xffff; __asm__ volatile( @@ -2497,9 +2498,9 @@ static inline void RENAME(hcscale_fast)(SwsContext *c, int16_t *dst, /* GCC 3.3 makes MPlayer crash on IA-32 machines when using "g" operand here, which is needed to support GCC 4.0. */ #if ARCH_X86_64 && AV_GCC_VERSION_AT_LEAST(3,4) - :: "m" (src1), "m" (dst), "g" (dstWidth), "m" (xInc_shr16), "m" (xInc_mask), + :: "m" (src1), "m" (dst), "g" (dstWidth_reg), "m" (xInc_shr16), "m" (xInc_mask), #else - :: "m" (src1), "m" (dst), "m" (dstWidth), "m" (xInc_shr16), "m" (xInc_mask), + :: "m" (src1), "m" (dst), "m" (dstWidth_reg), "m" (xInc_shr16), "m" (xInc_mask), #endif "r" (src2) : "%"REG_a, "%"REG_d, "%ecx", "%"REG_D, "%esi"
Tested on linux x86, x86_64, mingw32 x86, mingw-w64 x86_64, and ppc. All both with and without gpl.
Ramiro Polla
bfin/internal_bfin.S | 8 +- bfin/swscale_bfin.c | 8 +- colorspace-test.c | 2 rgb2rgb.c | 172 +++++++++++++++++++++++++-------------------------- rgb2rgb.h | 146 +++++++++++++++++++++---------------------- rgb2rgb_template.c | 154 ++++++++++++++++++++++----------------------- swscale.c | 20 ++--- swscale_internal.h | 18 ++--- swscale_template.c | 57 ++++++++-------- 9 files changed, 293 insertions(+), 292 deletions(-) 04410ad4397e17866d30731b02fab4728526f5f7 use_int.diff diff --git a/bfin/internal_bfin.S b/bfin/internal_bfin.S
many of the functions changed are used by applications directly (i remember them in mplayer) so this breaks ABI [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Good people do not need laws to tell them to act responsibly, while bad people will find a way around the laws. -- Plato
On Tue, Mar 2, 2010 at 7:16 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Feb 24, 2010 at 12:49:13AM -0300, Ramiro Polla wrote:
On Tue, Feb 2, 2010 at 11:48 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
if theres no speed difference i dont mind int being used
Patch attached. It changes all uses of long meant for width, height and stride to int. Besides renaming, it adds these chunks to hcscale_fast(): @@ -2469,6 +2469,7 @@ static inline void RENAME(hcscale_fast)(SwsContext *c, int16_t *dst, ? ? ? ? ?} ? ? ?} else { ?#endif /* COMPILE_TEMPLATE_MMX2 */ + ? ? ? ?x86_reg dstWidth_reg = dstWidth; ? ? ? ? ?x86_reg xInc_shr16 = (x86_reg) (xInc >> 16); ? ? ? ? ?uint16_t xInc_mask = xInc & 0xffff; ? ? ? ? ?__asm__ volatile( @@ -2497,9 +2498,9 @@ static inline void RENAME(hcscale_fast)(SwsContext *c, int16_t *dst, ?/* GCC 3.3 makes MPlayer crash on IA-32 machines when using "g" operand here, ?which is needed to support GCC 4.0. */ ?#if ARCH_X86_64 && AV_GCC_VERSION_AT_LEAST(3,4) - ? ? ? ? ? ?:: "m" (src1), "m" (dst), "g" (dstWidth), "m" (xInc_shr16), "m" (xInc_mask), + ? ? ? ? ? ?:: "m" (src1), "m" (dst), "g" (dstWidth_reg), "m" (xInc_shr16), "m" (xInc_mask), ?#else - ? ? ? ? ? ?:: "m" (src1), "m" (dst), "m" (dstWidth), "m" (xInc_shr16), "m" (xInc_mask), + ? ? ? ? ? ?:: "m" (src1), "m" (dst), "m" (dstWidth_reg), "m" (xInc_shr16), "m" (xInc_mask), ?#endif ? ? ? ? ? ? ?"r" (src2) ? ? ? ? ? ? ?: "%"REG_a, "%"REG_d, "%ecx", "%"REG_D, "%esi"
Tested on linux x86, x86_64, mingw32 x86, mingw-w64 x86_64, and ppc. All both with and without gpl.
Ramiro Polla
?bfin/internal_bfin.S | ? ?8 +- ?bfin/swscale_bfin.c ?| ? ?8 +- ?colorspace-test.c ? ?| ? ?2 ?rgb2rgb.c ? ? ? ? ? ?| ?172 +++++++++++++++++++++++++-------------------------- ?rgb2rgb.h ? ? ? ? ? ?| ?146 +++++++++++++++++++++---------------------- ?rgb2rgb_template.c ? | ?154 ++++++++++++++++++++++----------------------- ?swscale.c ? ? ? ? ? ?| ? 20 ++--- ?swscale_internal.h ? | ? 18 ++--- ?swscale_template.c ? | ? 57 ++++++++-------- ?9 files changed, 293 insertions(+), 292 deletions(-) 04410ad4397e17866d30731b02fab4728526f5f7 ?use_int.diff diff --git a/bfin/internal_bfin.S b/bfin/internal_bfin.S
many of the functions changed are used by applications directly (i remember them in mplayer) so this breaks ABI
Maybe MPlayer should stop using functions that aren't in the API... Anyways new patch attached doesn't change the rgb2rgb functions used by MPlayer.
On Saturday, 13 March 2010 at 00:29, Ramiro Polla wrote:
On Tue, Mar 2, 2010 at 7:16 AM, Michael Niedermayer <michaelni at gmx.at> wrote: [...]
many of the functions changed are used by applications directly (i remember them in mplayer) so this breaks ABI
Maybe MPlayer should stop using functions that aren't in the API...
It definitely should, patch welcome. ;) Regards, R. -- MPlayer http://mplayerhq.hu | RPMFusion http://rpmfusion.org There should be a science of discontent. People need hard times and oppression to develop psychic muscles. -- from "Collected Sayings of Muad'Dib" by the Princess Irulan
On Fri, Mar 12, 2010 at 08:29:32PM -0300, Ramiro Polla wrote:
many of the functions changed are used by applications directly (i remember them in mplayer) so this breaks ABI
Maybe MPlayer should stop using functions that aren't in the API...
MPlayer itself is _not_ an issue, we don't support using mismatching versions of libswscale anyway, the problem is that other programs might use them as well.
Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
On Fri, Mar 12, 2010 at 08:29:32PM -0300, Ramiro Polla wrote:
many of the functions changed are used by applications directly (i remember them in mplayer) so this breaks ABI
Maybe MPlayer should stop using functions that aren't in the API...
MPlayer itself is _not_ an issue, we don't support using mismatching versions of libswscale anyway, the problem is that other programs might use them as well.
If they're not in the public API, that's not our problem. -- M?ns Rullg?rd mans at mansr.com
On Sat, Mar 13, 2010 at 01:52:21PM +0000, M?ns Rullg?rd wrote:
Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
On Fri, Mar 12, 2010 at 08:29:32PM -0300, Ramiro Polla wrote:
many of the functions changed are used by applications directly (i remember them in mplayer) so this breaks ABI
Maybe MPlayer should stop using functions that aren't in the API...
MPlayer itself is _not_ an issue, we don't support using mismatching versions of libswscale anyway, the problem is that other programs might use them as well.
If they're not in the public API, that's not our problem.
Thats true and i agree but these functions are quite convenient to use the official API requires creating a full swscale context, keeping track of it and freeing ot again. This seems a bit overkill for a function that just reshuffles a few bits from rgb16 to rgb15 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Dictatorship naturally arises out of democracy, and the most aggravated form of tyranny and slavery out of the most extreme liberty. -- Plato
On Fri, Mar 12, 2010 at 8:29 PM, Ramiro Polla <ramiro.polla at gmail.com> wrote:
On Tue, Mar 2, 2010 at 7:16 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Feb 24, 2010 at 12:49:13AM -0300, Ramiro Polla wrote: [...]
?bfin/internal_bfin.S | ? ?8 +- ?bfin/swscale_bfin.c ?| ? ?8 +- ?colorspace-test.c ? ?| ? ?2 ?rgb2rgb.c ? ? ? ? ? ?| ?172 +++++++++++++++++++++++++-------------------------- ?rgb2rgb.h ? ? ? ? ? ?| ?146 +++++++++++++++++++++---------------------- ?rgb2rgb_template.c ? | ?154 ++++++++++++++++++++++----------------------- ?swscale.c ? ? ? ? ? ?| ? 20 ++--- ?swscale_internal.h ? | ? 18 ++--- ?swscale_template.c ? | ? 57 ++++++++-------- ?9 files changed, 293 insertions(+), 292 deletions(-) 04410ad4397e17866d30731b02fab4728526f5f7 ?use_int.diff diff --git a/bfin/internal_bfin.S b/bfin/internal_bfin.S
many of the functions changed are used by applications directly (i remember them in mplayer) so this breaks ABI
Maybe MPlayer should stop using functions that aren't in the API...
Anyways new patch attached doesn't change the rgb2rgb functions used by MPlayer.
Patch updated to latest SVN.
On Tue, May 04, 2010 at 07:29:09PM -0300, Ramiro Polla wrote:
On Fri, Mar 12, 2010 at 8:29 PM, Ramiro Polla <ramiro.polla at gmail.com> wrote:
On Tue, Mar 2, 2010 at 7:16 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Feb 24, 2010 at 12:49:13AM -0300, Ramiro Polla wrote: [...]
?bfin/internal_bfin.S | ? ?8 +- ?bfin/swscale_bfin.c ?| ? ?8 +- ?colorspace-test.c ? ?| ? ?2 ?rgb2rgb.c ? ? ? ? ? ?| ?172 +++++++++++++++++++++++++-------------------------- ?rgb2rgb.h ? ? ? ? ? ?| ?146 +++++++++++++++++++++---------------------- ?rgb2rgb_template.c ? | ?154 ++++++++++++++++++++++----------------------- ?swscale.c ? ? ? ? ? ?| ? 20 ++--- ?swscale_internal.h ? | ? 18 ++--- ?swscale_template.c ? | ? 57 ++++++++-------- ?9 files changed, 293 insertions(+), 292 deletions(-) 04410ad4397e17866d30731b02fab4728526f5f7 ?use_int.diff diff --git a/bfin/internal_bfin.S b/bfin/internal_bfin.S
many of the functions changed are used by applications directly (i remember them in mplayer) so this breaks ABI
Maybe MPlayer should stop using functions that aren't in the API...
Anyways new patch attached doesn't change the rgb2rgb functions used by MPlayer.
Patch updated to latest SVN.
do what you want with the int <-> long as long as it has been benchmarked and is not slower on x86_64 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I hate to see young programmers poisoned by the kind of thinking Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
On Tue, May 4, 2010 at 9:31 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, May 04, 2010 at 07:29:09PM -0300, Ramiro Polla wrote:
On Fri, Mar 12, 2010 at 8:29 PM, Ramiro Polla <ramiro.polla at gmail.com> wrote:
On Tue, Mar 2, 2010 at 7:16 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Feb 24, 2010 at 12:49:13AM -0300, Ramiro Polla wrote: [...]
?bfin/internal_bfin.S | ? ?8 +- ?bfin/swscale_bfin.c ?| ? ?8 +- ?colorspace-test.c ? ?| ? ?2 ?rgb2rgb.c ? ? ? ? ? ?| ?172 +++++++++++++++++++++++++-------------------------- ?rgb2rgb.h ? ? ? ? ? ?| ?146 +++++++++++++++++++++---------------------- ?rgb2rgb_template.c ? | ?154 ++++++++++++++++++++++----------------------- ?swscale.c ? ? ? ? ? ?| ? 20 ++--- ?swscale_internal.h ? | ? 18 ++--- ?swscale_template.c ? | ? 57 ++++++++-------- ?9 files changed, 293 insertions(+), 292 deletions(-) 04410ad4397e17866d30731b02fab4728526f5f7 ?use_int.diff diff --git a/bfin/internal_bfin.S b/bfin/internal_bfin.S
many of the functions changed are used by applications directly (i remember them in mplayer) so this breaks ABI
Maybe MPlayer should stop using functions that aren't in the API...
Anyways new patch attached doesn't change the rgb2rgb functions used by MPlayer.
Patch updated to latest SVN.
do what you want with the int <-> long as long as it has been benchmarked and is not slower on x86_64
Applied.
On Mon, May 10, 2010 at 9:37 PM, Ramiro Polla <ramiro.polla at gmail.com> wrote:
On Tue, May 4, 2010 at 9:31 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, May 04, 2010 at 07:29:09PM -0300, Ramiro Polla wrote:
On Fri, Mar 12, 2010 at 8:29 PM, Ramiro Polla <ramiro.polla at gmail.com> wrote:
On Tue, Mar 2, 2010 at 7:16 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Feb 24, 2010 at 12:49:13AM -0300, Ramiro Polla wrote: [...]
?bfin/internal_bfin.S | ? ?8 +- ?bfin/swscale_bfin.c ?| ? ?8 +- ?colorspace-test.c ? ?| ? ?2 ?rgb2rgb.c ? ? ? ? ? ?| ?172 +++++++++++++++++++++++++-------------------------- ?rgb2rgb.h ? ? ? ? ? ?| ?146 +++++++++++++++++++++---------------------- ?rgb2rgb_template.c ? | ?154 ++++++++++++++++++++++----------------------- ?swscale.c ? ? ? ? ? ?| ? 20 ++--- ?swscale_internal.h ? | ? 18 ++--- ?swscale_template.c ? | ? 57 ++++++++-------- ?9 files changed, 293 insertions(+), 292 deletions(-) 04410ad4397e17866d30731b02fab4728526f5f7 ?use_int.diff diff --git a/bfin/internal_bfin.S b/bfin/internal_bfin.S
many of the functions changed are used by applications directly (i remember them in mplayer) so this breaks ABI
Maybe MPlayer should stop using functions that aren't in the API...
Anyways new patch attached doesn't change the rgb2rgb functions used by MPlayer.
Patch updated to latest SVN.
do what you want with the int <-> long as long as it has been benchmarked and is not slower on x86_64
Applied.
I'm finally back on this issue. I had committed and then reverted this because there were some failures. My revert commit log reads: "Revert r31153. It failed to build on: x86_64 / Mac OS X gcc 4.0.1 x86_64 / Linux icc (all) x86_64 / Linux gcc 4.0.4 x86_64 / OpenBSD gcc 3.3.5 x86_64 / Linux suncc 5.10 and there are some reports of crashes." Unfortunately I don't remember where the crash reports were (IIRC dark shikari pointed me to one on irc and there was one that seemed related on roundup, but I could not find it). I did catch some crashes with the intel 64-bit compiler and fixed them. Attached patch was tested to build and run swscale-test on: i686-pc-mingw32 gcc 4.4.4 i686-w64-mingw32 gcc 4.4.4 x86_64-w64-mingw32 gcc 4.4.4 suncc 5.10 gcc 4.4.3-4ubuntu5 (32-bit, 64-bit, and with asm disabled) llvm64 2.7 icc 11.1 (32- and 64-bit) Differences from the previous patch are more uses of "x86_reg dstW_reg;" and similar. Just look for "_reg" in the patch, the rest is the same as the previous one. Ramiro Polla
On Sat, Jul 17, 2010 at 9:23 PM, Ramiro Polla <ramiro.polla at gmail.com> wrote:
On Mon, May 10, 2010 at 9:37 PM, Ramiro Polla <ramiro.polla at gmail.com> wrote:
On Tue, May 4, 2010 at 9:31 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Tue, May 04, 2010 at 07:29:09PM -0300, Ramiro Polla wrote:
On Fri, Mar 12, 2010 at 8:29 PM, Ramiro Polla <ramiro.polla at gmail.com> wrote:
On Tue, Mar 2, 2010 at 7:16 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Wed, Feb 24, 2010 at 12:49:13AM -0300, Ramiro Polla wrote: [...] > ?bfin/internal_bfin.S | ? ?8 +- > ?bfin/swscale_bfin.c ?| ? ?8 +- > ?colorspace-test.c ? ?| ? ?2 > ?rgb2rgb.c ? ? ? ? ? ?| ?172 +++++++++++++++++++++++++-------------------------- > ?rgb2rgb.h ? ? ? ? ? ?| ?146 +++++++++++++++++++++---------------------- > ?rgb2rgb_template.c ? | ?154 ++++++++++++++++++++++----------------------- > ?swscale.c ? ? ? ? ? ?| ? 20 ++--- > ?swscale_internal.h ? | ? 18 ++--- > ?swscale_template.c ? | ? 57 ++++++++-------- > ?9 files changed, 293 insertions(+), 292 deletions(-) > 04410ad4397e17866d30731b02fab4728526f5f7 ?use_int.diff > diff --git a/bfin/internal_bfin.S b/bfin/internal_bfin.S
many of the functions changed are used by applications directly (i remember them in mplayer) so this breaks ABI
Maybe MPlayer should stop using functions that aren't in the API...
Anyways new patch attached doesn't change the rgb2rgb functions used by MPlayer.
Patch updated to latest SVN.
do what you want with the int <-> long as long as it has been benchmarked and is not slower on x86_64
Applied.
I'm finally back on this issue. I had committed and then reverted this because there were some failures. My revert commit log reads: "Revert r31153. It failed to build on: x86_64 / Mac OS X gcc 4.0.1 x86_64 / Linux icc (all) x86_64 / Linux gcc 4.0.4 x86_64 / OpenBSD gcc 3.3.5 x86_64 / Linux suncc 5.10 and there are some reports of crashes."
Unfortunately I don't remember where the crash reports were (IIRC dark shikari pointed me to one on irc and there was one that seemed related on roundup, but I could not find it). I did catch some crashes with the intel 64-bit compiler and fixed them.
Attached patch was tested to build and run swscale-test on: i686-pc-mingw32 gcc 4.4.4 i686-w64-mingw32 gcc 4.4.4 x86_64-w64-mingw32 gcc 4.4.4 suncc 5.10 gcc 4.4.3-4ubuntu5 (32-bit, 64-bit, and with asm disabled) llvm64 2.7 icc 11.1 (32- and 64-bit)
Differences from the previous patch are more uses of "x86_reg dstW_reg;" and similar. Just look for "_reg" in the patch, the rest is the same as the previous one.
A patch by anton was applied by jason and no one seemed to mind the casts (they were pretty much the same as in my patch, minus the long->int change). I think the long->int change (and this patch) can be dropped if people are ok with the casts.
Ramiro Polla <ramiro.polla at gmail.com> writes:
Index: libavutil/common.h =================================================================== --- libavutil/common.h (revision 21602) +++ libavutil/common.h (working copy) @@ -337,6 +337,28 @@
#include "mem.h"
+/** + * The array_index type is the same width as the registers the architecture + * uses to index arrays,
What does that mean? Indexing arrays is not a concept that exists at the machine level.
which for example simplifies calculations without + * requiring a 32->64 bit conversion. In this example code compilers will use + * width as both the counter and the array index itself, instead of creating + * one more variable. + * @code + * func(int *dst, int *src, array_index width) { + * int i; + * for (i = 0; i < width; i++) + * dst[i] = src[2*i]; + * }
Regardless of the type, the compiler is just as likely to turn that into something resembling this (yes, I've seen them do it): while (width-- > 0) { *dst++ = *src; src += 2; } Now there's no indexing at all, and no reason for anything to have a special type. For all you know it could even get worse. In fact, on x86_64 insisting on a 64-bit type there means anything touching it will need a 64-bit prefix thus increasing the code size for no good reason at all.
+ * @endcode + */ +#if ARCH_X86_64 +typedef int64_t array_index; +#elif ARCH_X86_32 +typedef int32_t array_index; +#else +typedef int array_index; +#endif
Under no circumstance will I allow this in a public header. -- M?ns Rullg?rd mans at mansr.com
participants (7)
-
dominik＠rangers.eu.org -
kostya.shishkov＠gmail.com -
mans＠mansr.com -
michaelni＠gmx.at -
ramiro.polla＠gmail.com -
Reimar.Doeffinger＠gmx.de -
zuxy.meng＠gmail.com