Change prototypes to yuv_convert_simd_x86 yasm and equivalent C to portably sign extend int args

Change prototypes to yuv_convert_simd_x86 yasm and equivalent C to portably sign extend int args

Inspection of stack prepared by caller shows it is indeed copying dword, not qword, for 32-bit arguments on Win64, and the background stack is not zeroed.

Incorrect function result, access violations or worse (not-immediately caught side effect of corrupted memory within process address space) could easily result from current win64 usage of these routines.

One solution, implemented here, is to change the function prototypes for the yasm routines to take ptrdiff_t arguments instead of int args.  The C version of the equivalent code needs matching prototype (due to the choose proc routines), so they are updated too.

Another, probably slightly less performant, solution would be to change the asm routines to conditionally sign-extend when loading int (32-bit) parameters on x64.  None of these routines currently use int passed in directly as register; I  don't know if zero-extension of registers would be required if they did.

Yet another solution would be to switch to VS2012 and redo the yasm routines using compiler intrinsics completely (not supported in VS2010).

Note, some other media_unittests, namely those involving ffmpeg, currently give access violation on Win64.  There is a separate ffmpeg fix in progress https://gerrit.chromium.org/gerrit/#/c/43063/.  With PS1 or PS2 applied along with the ffmpeg fix, Win64 media_unittests pass.

Detail:
The following are sign-extended in prototype:
simd/convert_rgb_to_yuv_ssse3.asm:%define SYMBOL          ConvertARGBToYUVRow_SSSE3
simd/convert_rgb_to_yuv_ssse3.asm:%define SYMBOL          ConvertRGBToYUVRow_SSSE3
simd/convert_yuv_to_rgb_mmx.asm:%define SYMBOL ConvertYUVToRGB32Row_MMX
simd/convert_yuv_to_rgb_sse.asm:%define SYMBOL ConvertYUVToRGB32Row_SSE
simd/scale_yuv_to_rgb_mmx.asm:%define SYMBOL ScaleYUVToRGB32Row_MMX
simd/scale_yuv_to_rgb_sse.asm:%define SYMBOL ScaleYUVToRGB32Row_SSE
simd/scale_yuv_to_rgb_sse2_x64.asm:%define SYMBOL ScaleYUVToRGB32Row_SSE2_X64
simd/linear_scale_yuv_to_rgb_mmx.asm:%define SYMBOL LinearScaleYUVToRGB32Row_MMX
simd/linear_scale_yuv_to_rgb_sse.asm:%define SYMBOL LinearScaleYUVToRGB32Row_SSE
simd/linear_scale_yuv_to_rgb_mmx_x64.asm:%define SYMBOL LinearScaleYUVToRGB32Row_MMX_X64

The following C versions and the corresponding typedefs also needed updating to match:
ConvertYUVToRGB32Row_C
ScaleYUVToRGB32Row_C
LinearScaleYUVToRGB32Row_C

The following yasm routines have no C prototype, nor do they appear to be used anywhere.  I updated their comments though to match the same prototype change that would be necessary.
simd/convert_rgb_to_yuv_ssse3.asm:%define SYMBOL          ConvertARGBToYUVEven_SSSE3
simd/convert_rgb_to_yuv_ssse3.asm:%define SYMBOL          ConvertARGBToYUVOdd_SSSE3
simd/convert_rgb_to_yuv_ssse3.asm:%define SYMBOL          ConvertRGBToYUVEven_SSSE3
simd/convert_rgb_to_yuv_ssse3.asm:%define SYMBOL          ConvertRGBToYUVOdd_SSSE3

BUG=174160
TEST=with https://gerrit.chromium.org/gerrit/#/c/43063/ also applied, all win64 media_unittests non-disabled test cases pass
R=hclam@chromium.org
R=dalecurtis@chromium.org
R=rbultje@chromium.org
CC=jschuh@chromium.org
CC=scherkus@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182407

[wolenetz, 2013-02-07 06:04:34]

Please review.

Thanks,
Matt

[DaleCurtis, 2013-02-07 21:12:13]

On 2013/02/07 06:04:34, wolenetz wrote:
> Please review.
> 
> Thanks,
> Matt

Do Ronald's comments via email change your approach here?

[wolenetz, 2013-02-07 21:31:06]

On 2013/02/07 21:12:13, DaleCurtis wrote:
> On 2013/02/07 06:04:34, wolenetz wrote:
> > Please review.
> > 
> > Thanks,
> > Matt
> 
> Do Ronald's comments via email change your approach here?

Very possible.  I will need to look into whether ptrdiff_t (or intptr_t) is
portable.  Alternatively, I could look into whether his example of movsxdifnidn
is similarly portable (though claimed as less efficient).  Either way, I do not
recommend this current PS1 be committed (it smells of brittleness.)  Please do
comment on any approach I may have missed though :)

[wolenetz, 2013-02-12 19:48:22]

Please review PS2.
rbultje@, dalecurtis@ and hclam@: please review all pieces of PS2.

Thanks!
Matt

[wolenetz, 2013-02-12 19:52:12]

I'll pend an upload of PS3 with the whitespace fix on any other review comments
on PS2.

https://codereview.chromium.org/12211067/diff/4001/media/base/simd/convert_yu...
File media/base/simd/convert_yuv_to_rgb.h (right):

https://codereview.chromium.org/12211067/diff/4001/media/base/simd/convert_yu...
media/base/simd/convert_yuv_to_rgb.h:68: 
I'll remove this extra space in PS3.  Sorry this one crept through!

[DaleCurtis, 2013-02-12 19:52:33]

ooc, I don't suppose size_t is zero extended? it would make a more logical
choice.

[wolenetz, 2013-02-12 20:02:21]

On 2013/02/12 19:52:33, DaleCurtis wrote:
> ooc, I don't suppose size_t is zero extended? it would make a more logical
> choice.

I have a couple not very strong points here, supporting ptrdiff_t:
1. size_t is unsigned.  int and ptrdiff_t are signed. yasm callers may
(incorrectly) have negative values, and behavior of callee between size_t cast
vs sign-extend could be different.  Ideally, the compiler would error on attempt
to pass (without explicit cast/conversion) a signed type into an unsigned
parameter.
2. ffmpeg yasm C prototypes already use ptrdiff_t all over the place for similar
purpose.

[wolenetz, 2013-02-12 20:53:43]

On 2013/02/12 20:02:21, wolenetz wrote:
> On 2013/02/12 19:52:33, DaleCurtis wrote:
> > ooc, I don't suppose size_t is zero extended? it would make a more logical
> > choice.
> 
> I have a couple not very strong points here, supporting ptrdiff_t:
> 1. size_t is unsigned.  int and ptrdiff_t are signed. yasm callers may
> (incorrectly) have negative values, and behavior of callee between size_t cast
> vs sign-extend could be different.  Ideally, the compiler would error on
attempt
> to pass (without explicit cast/conversion) a signed type into an unsigned
> parameter.
> 2. ffmpeg yasm C prototypes already use ptrdiff_t all over the place for
similar
> purpose.

Still not a very strong point, but attempt to use size_t demonstrated that, with
current code:
1. More C4267 suppressions in yuv_convert_simd_x86 target would be needed on
Win64 (due to C-equivalent routines working internally with ints truncated from
size_t parameters), and
2. On linux, comparison of int vs unsigned size_t occurs, causing compiler
errors in those C-equivalent routines)

Both are fixable.  The question stands: is size_t or ptrdiff_t best, (or do this
all conditionally in the asm from int)?

[rbultje1, 2013-02-12 21:39:44]

ptrdiff_t is syntactically better, since you're incrementing a pointer by a
number that is a relative offset to that address and another address (e.g. end
of buffer, end of line), i.e. a difference between pointers, i.e. ptrdiff_t.

In practical terms, size_t may work also but I'm not sure it always expands to
the correct thing on each platform. That is, I know ptrdiff_t solves our problem
and has no side-effects, I don't know for sure if that's the case for size_t.

[DaleCurtis, 2013-02-12 21:42:05]

this lgtm to me given the above arguments.

[wolenetz, 2013-02-12 22:30:05]

PS3 is uploaded.
hclam@ and rbultje@: please review all

Thanks!
Matt

https://codereview.chromium.org/12211067/diff/4001/media/base/simd/convert_yu...
File media/base/simd/convert_yuv_to_rgb.h (right):

https://codereview.chromium.org/12211067/diff/4001/media/base/simd/convert_yu...
media/base/simd/convert_yuv_to_rgb.h:68: 
On 2013/02/12 19:52:12, wolenetz wrote:
> I'll remove this extra space in PS3.  Sorry this one crept through!

Done.

[rbultje1, 2013-02-12 22:35:03]

how are the *_x64 files different from the non-*_x64 files?

[wolenetz, 2013-02-12 23:23:12]

On 2013/02/12 22:35:03, rbultje1 wrote:
> how are the *_x64 files different from the non-*_x64 files?

Alpha can say for sure, but the _x64 versions are optimized for X64, while the
others are not.  Their prototypes are typedef'ed in common, so they share the
ptrdiff_t change even on non-_x64 (or in case of non-x86-family, in the
C-equivalent version).

[Alpha Left Google, 2013-02-13 00:14:24]

The _x64 files are specifically optimized for x64 that use more registers or xmm
registers. They are already used in linux x64 builds.

[wolenetz, 2013-02-13 18:25:52]

On 2013/02/13 00:14:24, Alpha wrote:
> The _x64 files are specifically optimized for x64 that use more registers or
xmm
> registers. They are already used in linux x64 builds.

Ronald and Alpha, are there any concerns you have on this CL?  I'd like to get
this committed to enable win64 media_unittests from tree.

[Alpha Left Google, 2013-02-13 18:28:40]

LGTM.

[rbultje1, 2013-02-13 18:29:45]

lgtm too.

[commit-bot: I haz the power, 2013-02-13 18:31:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/12211067/11001

[commit-bot: I haz the power, 2013-02-13 19:10:44]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-13 19:44:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/12211067/11001

[commit-bot: I haz the power, 2013-02-14 07:33:00]

Change committed as 182407