[FFmpeg-devel] [PATCH] MMX2 scaler: Determine "funnyCode" size at runtime.
Michael Niedermayer
michaelni
Thu Aug 13 17:52:33 CEST 2009
On Thu, Aug 13, 2009 at 10:16:42AM -0300, Ramiro Polla wrote:
> 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
> swscale.c | 48 ++++++++++++++++++++++++------------------------
> swscale_internal.h | 4 ++--
> swscale_template.c | 48 ++++++++++++++++++++++++------------------------
> 3 files changed, 50 insertions(+), 50 deletions(-)
> 1b08ea25b1cbdf674c7ec74a0cb4d253f9dbcd0b 0001-Rename-funny-code-to-mmx2-filter-code.patch
> From 8f1574928841b813762e353bff74ff79de1415b7 Mon Sep 17 00:00:00 2001
> From: Ramiro Polla <ramiro.polla at gmail.com>
> Date: Thu, 13 Aug 2009 09:17:58 -0300
> Subject: [PATCH] Rename "funny" code to "mmx2 filter" code.
>
> ---
> swscale.c | 48 ++++++++++++++++++++++++------------------------
> swscale_internal.h | 4 ++--
> swscale_template.c | 48 ++++++++++++++++++++++++------------------------
> 3 files changed, 50 insertions(+), 50 deletions(-)
ok
[...]
> swscale_template.c | 21 ---------------------
> 1 file changed, 21 deletions(-)
> 2e51d5790599b947b3ea076a7d0eb0a41a1048df 0002-Remove-duplicate-define-it-is-the-same-in-the-lum-c.patch
> From f3c2dcdf18c86ca1a7b575717517d95e0d9d75b6 Mon Sep 17 00:00:00 2001
> From: Ramiro Polla <ramiro.polla at gmail.com>
> Date: Thu, 13 Aug 2009 09:23:19 -0300
> Subject: [PATCH] Remove duplicate define (it is the same in the lum code).
>
> ---
> swscale_template.c | 21 ---------------------
> 1 files changed, 0 insertions(+), 21 deletions(-)
ok
[...]
> swscale.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
> 2f866ea7962f17cd314ff4a4ad9dfc04f709bf7f 0003-Protect-mmx2-filter-code-buffers-so-they-are-not-exe.patch
> From ec223459ec23e1208a2c8a76f95ec885c1a30b68 Mon Sep 17 00:00:00 2001
> From: Ramiro Polla <ramiro.polla at gmail.com>
> Date: Thu, 13 Aug 2009 09:41:51 -0300
> Subject: [PATCH] Protect mmx2 filter code buffers so they are not executable and writeable at
> the same time (only mmap for now).
>
> ---
> swscale.c | 9 +++++++--
> 1 files changed, 7 insertions(+), 2 deletions(-)
ok
[...]
> swscale.c | 2 ++
> 1 file changed, 2 insertions(+)
> 9cb63462d2b442d2a7768b1a2a15de93d7cb656f 0004-MMX2-horizontal-scaler-Comment-differences-between.patch
> From 7077c874181e2461f2985b28495a7df08ef49522 Mon Sep 17 00:00:00 2001
> From: Ramiro Polla <ramiro.polla at gmail.com>
> Date: Thu, 13 Aug 2009 09:53:27 -0300
> Subject: [PATCH] MMX2 horizontal scaler: Comment differences between fragment A and fragment B.
>
> ---
> swscale.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/swscale.c b/swscale.c
> index 4a69ab8..c73039e 100644
> --- a/swscale.c
> +++ b/swscale.c
> @@ -1771,6 +1771,8 @@ static void initMMX2HScaler(int dstW, int xInc, uint8_t *filterCode, int16_t *fi
>
> //code fragment
>
> + /* Fragment A reads 8 samples to perform the filtering, whereas fragment B
> + * reads only 4 samples. Besides that, both fragments are equal. */
> __asm__ volatile(
this description is poor and not really correct
[...]
> swscale.c | 36 ++++++++++++++++++++++--------------
> swscale_internal.h | 2 ++
> 2 files changed, 24 insertions(+), 14 deletions(-)
> 77be7f55d39f8c95471c195b4b2c439c79d482e1 0005-MMX2-horizontal-scaler-Determine-code-size-at-runti.patch
> From cae79c746493d4c9759a326eb646b007b8303023 Mon Sep 17 00:00:00 2001
> From: Ramiro Polla <ramiro.polla at gmail.com>
> Date: Thu, 13 Aug 2009 10:03:00 -0300
> Subject: [PATCH] MMX2 horizontal scaler: Determine code size at runtime.
>
> ---
> swscale.c | 36 ++++++++++++++++++++++--------------
> swscale_internal.h | 2 ++
> 2 files changed, 24 insertions(+), 14 deletions(-)
ok
[...]
--
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090813/d976e16c/attachment.pgp>
More information about the ffmpeg-devel
mailing list