[FFmpeg-devel] [PATCH] MMX2 scaler: Determine "funnyCode" size at runtime.

[Ramiro Polla]

Hi Michael,

On Wed, Aug 12, 2009 at 10:38 PM, Michael Niedermayer<michaelni at gmx.at> wrote:
> On Wed, Aug 12, 2009 at 09:38:00PM -0300, Ramiro Polla wrote:
>> Hi,
>>
>> $subj again.
>>
>> Ramiro Polla
>
>> ?swscale.c ? ? ? ? ?| ? 49 ++++++++++++++++++++++++++++++-------------------
>> ?swscale_internal.h | ? ?2 ++
>> ?2 files changed, 32 insertions(+), 19 deletions(-)
>> d69a3a913f17ec8d300b681bb8b41b2f7ef92576 ?0001-MMX2-scaler-Determine-funnyCode-size-at-runtime.patch
>> From 49139b8e6daf70ad595698279406753cbaf40d98 Mon Sep 17 00:00:00 2001
>> From: Ramiro Polla <ramiro.polla at gmail.com>
>> Date: Mon, 27 Jul 2009 04:47:30 -0300
>> Subject: [PATCH] MMX2 scaler: Determine "funnyCode" size at runtime.
>>
>> ---
>> ?swscale.c ? ? ? ? ?| ? 49 ++++++++++++++++++++++++++++++-------------------
>> ?swscale_internal.h | ? ?2 ++
>> ?2 files changed, 32 insertions(+), 19 deletions(-)
>>
>> diff --git a/swscale.c b/swscale.c
>> index 2adf393..6e0535c 100644
>> --- a/swscale.c
>> +++ b/swscale.c
>> @@ -1753,7 +1753,7 @@ error:
>> ?}
>>
>> ?#ifdef COMPILE_MMX2
>> -static void initMMX2HScaler(int dstW, int xInc, uint8_t *funnyCode, int16_t *filter, int32_t *filterPos, int numSplits)
>> +static void initMMX2HScaler(int dstW, int xInc, uint8_t **funnyCodePtr, int *funnyCodeSizePtr, int16_t *filter, int32_t *filterPos, int numSplits)
>> ?{
>> ? ? ?uint8_t *fragmentA;
>> ? ? ?x86_reg imm8OfPShufW1A;
>> @@ -1763,6 +1763,8 @@ static void initMMX2HScaler(int dstW, int xInc, uint8_t *funnyCode, int16_t *fil
>> ? ? ?x86_reg imm8OfPShufW1B;
>> ? ? ?x86_reg imm8OfPShufW2B;
>> ? ? ?x86_reg fragmentLengthB;
>> + ? ?int funnyCodeSize = 1;
>> + ? ?uint8_t *funnyCode;
>> ? ? ?int fragmentPos;
>>
>> ? ? ?int xpos, i;
>
>> @@ -1852,6 +1854,27 @@ static void initMMX2HScaler(int dstW, int xInc, uint8_t *funnyCode, int16_t *fil
>> ? ? ?xpos= 0; //lumXInc/2 - 0x8000; // difference between pixel centers
>> ? ? ?fragmentPos=0;
>>
>> + ? ?/* Determine code size. */
>> + ? ?for (i=0; i<dstW/numSplits; i+=4) {
>> + ? ? ? ?if (((xpos+xInc*3)>>16) - (xpos>>16) < 3)
>> + ? ? ? ? ? ?funnyCodeSize += fragmentLengthB;
>> + ? ? ? ?else
>> + ? ? ? ? ? ?funnyCodeSize += fragmentLengthA;
>> + ? ? ? ?xpos+=xInc*4;
>> + ? ?}
>> + ? ?*funnyCodeSizePtr = funnyCodeSize;
>
> this is obfuscated
> someone changng some related code would almost certainly miss updating this

> also *A/*B are very poor names

It's your code =). I don't think they're that bad, just that some
documentation would make it much clearer. Otherwise the variable name
would have to grow from fragmentA to something like fragmentRead4 and
it still wouldn't make it as clear as the comment that this is the
only difference between them. I attached a patch with the comment, but
feel free to suggest better variable names.

And since we're touching the "very poor [variable] names" subject,
what about "funny" code -> "mmx2 filter"? Patch attached (hmm, all
other patches depend on this one).

> i think the safest way is to just run the code twice and in the first run dont
> write anything, just count how much would be written

Patch attached.

>> +
>> +#ifdef MAP_ANONYMOUS
>> + ? ?*funnyCodePtr = mmap(NULL, funnyCodeSize, PROT_EXEC | PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
>> +#elif HAVE_VIRTUALALLOC
>> + ? ?*funnyCodePtr = VirtualAlloc(NULL, funnyCodeSize, MEM_COMMIT, PAGE_EXECUTE_READWRITE);
>> +#else
>> + ? ?*funnyCodePtr = av_malloc(funnyCodeSize);
>> +#endif
>> + ? ?funnyCode = *funnyCodePtr;
>> +
>> + ? ?xpos= 0;
>> +
>
> memory should never be writeable and executable,
> making it writeable, write into it and then switch from write to executeable
> is what should be done.

Patch attached. I'll work on the Windows case if/when this one gets committed.

Ramiro Polla
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Rename-funny-code-to-mmx2-filter-code.patch
Type: text/x-diff
Size: 10995 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090813/5acee411/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Remove-duplicate-define-it-is-the-same-in-the-lum-c.patch
Type: text/x-diff
Size: 1509 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090813/5acee411/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Protect-mmx2-filter-code-buffers-so-they-are-not-exe.patch
Type: text/x-diff
Size: 2146 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090813/5acee411/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-MMX2-horizontal-scaler-Comment-differences-between.patch
Type: text/x-diff
Size: 816 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090813/5acee411/attachment-0003.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-MMX2-horizontal-scaler-Determine-code-size-at-runti.patch
Type: text/x-diff
Size: 5926 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090813/5acee411/attachment-0004.patch>