Assembly Optimization

[MysteryX]

I have very simple code to convert pixels from 8-bit to 16-bit integers here
https://github.com/mysteryx93/AviSynthShader/blob/master/Src/ConvertToFloat.cpp#L199

void ConvertToFloat::convInt(unsigned char y, unsigned char u, unsigned char v, unsigned char* out) {
if (precision == 1) {
out[0] = y;
out[1] = u;
out[2] = v;
}
else { // Precision == 2
unsigned short outY = y << 8;
unsigned short outU = u << 8;
unsigned short outV = v << 8;
memcpy(out, &outY, 2);
memcpy(out + 2, &outU, 2);
memcpy(out + 4, &outV, 2);
}
}

This should be pretty easy to optimize with assembly code. I believe the way it works is that it has to be written to process 16 pixels at a time?

Is there someone with knowledge of assembly who could do this?

The simplest way to develop and test this would be to download the project files, keep only Init.cpp, ConvertToFloat and ConvertFromFloat, then test this scripts, with precision=1 and precision=2. It should display back the source back.

ConvertToFloat(precision=2)
ConvertFromFloat(precision=2)

[jpsdr]

For this, you need a function which convert a least a whole line, not only one value, and in that case, you problably can use SSE2 instructions which can process several pixels at the same time. But, i'm affraid that if you stay with only one pixel at the time, no big things can be done. But calling memcpy each time is very slow, i suggest you try this :


void ConvertToFloat::convInt(unsigned char y, unsigned char u, unsigned char v, void *out_) {
if (precision == 1) {
unsigned char *out=(unsigned char *)out_;
out[0] = y;
out[1] = u;
out[2] = v;
}
else { // Precision == 2
unsigned short *out=(unsigned short *)out_;
out[0]= (unsigned short)y;
out[1]= (unsigned short)u;
out[2]= (unsigned short)v;
}
}


just doing this will probably be faster.

EDIT : Ouups, don't need to shift in that case for 16 bit values.... Shift was specific to memory transfert, and endianess of Intel CPU. Unless, if converting to 16bits you want also to multiply value by 256... ;)

[foxyshadis]

Check out fmtconv's templates (https://github.com/EleonoreMizo/fmtconv/blob/master/src/fmtcl/BitBltConv.cpp#L745) for a solid implementation, though you have to plow through the logic of the proxy pointer and tools. There's also AVX versions in another file. Of course in VS, you could just call fmtconv instead if doing it yourself.

[ndjamena]

Would a union not work?

-edit- oh, right, you might as well just use an array of six chars...


unsigned char *out=(unsigned char *)out_;
out[0] = y;
out[1] = out[3] = out[5] = '\0';
out[2] = u;
out[4] = v;



struct Boo
{
char a1;
short a2;
short a3;
char a4;
};
Boo *out=(Boo *)out_;
out->a1 = '\0';
out->a2 = y;
out->a3 = u;
out->a4 = v;

[MysteryX]

For this, you need a function which convert a least a whole line, not only one value
Yeah, the function calling it would have to be rewritten too.

Basically, I have to loop through the 3 planes of YV24 perform "<< 8" on each value, and combine all 3 planes into 1 plane with UINT16 data.

There there are a few other variants: instead doing it from RGB32, performing the reverse, and doing all those with BYTE data.

Check out fmtconv's templates (https://github.com/EleonoreMizo/fmtconv/blob/master/src/fmtcl/BitBltConv.cpp#L745) for a solid implementation, though you have to plow through the logic of the proxy pointer and tools. There's also AVX versions in another file. Of course in VS, you could just call fmtconv instead if doing it yourself.
What does that function do exactly?

[foxyshadis]

What does that function do exactly?

Converting one integer format to another; you can instantiate the template with 8bit source and 16bit dest. (There are others in the same file that do int-to-float and vice versa.)

Here is that function rewritten strictly for your use case, with raw pointers:

// restrictions: src_stride MUST BE a multiple of 8, dst_stride MUST BE a multiple of 16 and at least 2x src_stride

void bitblt_i8_to_i16_sse2 (uint16_t* dst_ptr, int dst_stride, const uint8_t* src_ptr, int src_stride, int h)
{
assert (src_stride % 8 == 0);
assert (dst_stride / src_stride >= 2);

const __m128i zero = _mm_setzero_si128 ();
const __m128i val_ma = _mm_set1_epi16 (0);
const __m128i mask_lsb = _mm_set1_epi16 (0x00FF);

for (int y = 0; y < h; ++y)
{
uint16_t* cur_dst_ptr = dst_ptr;
const uint8_t* cur_src_ptr = src_ptr;

for (int x = 0; x < src_stride; x += 8)
{
__m128i val = load_8_16l (cur_src_ptr, zero);
val = _mm_slli_epi16 (val, 8);

store_8_16l(cur_dst_ptr, val, mask_lsb);

cur_src_ptr += 8;
cur_dst_ptr += 8;
}
}
}

__m128i load_8_16l (const void *lsb_ptr, __m128i zero)
{
assert (lsb_ptr != 0);

const __m128i val_lsb = _mm_loadl_epi64 (
reinterpret_cast <const __m128i *> (lsb_ptr)
);
const __m128i val = _mm_unpacklo_epi8 (val_lsb, zero);

return (val);
}

void store_8_16l (void *lsb_ptr, __m128i val, __m128i mask_lsb)
{
assert (lsb_ptr != 0);

__m128i lsb = _mm_and_si128 (mask_lsb, val);
lsb = _mm_packus_epi16 (lsb, lsb);
_mm_storel_epi64 (reinterpret_cast <__m128i *> (lsb_ptr), lsb);
}

If you want to loosen the restrictions, you can add a final leftover pixels loop after the main loop (as fmtconv does), but slightly widening the stride in exchange for foregoing the leftover loop is a good speed trade-off. Avisynth should never have a stride less than 8, and you control the destination's stride. You could probably force inline of both of those helper functions; I haven't tested to see how much it would help. I can do the 16->8 if you need that too. (Though sticking to 16 and requiring DitherTools to convert to stack16 or 8 might be better, since that gives you high quality dither as well.)

[TheFluff]

if the starting point is memcpy'ing two bytes at a time perhaps taking some time to explain function calls and their overhead would be in order before covering more advanced material

matt 7:6

[MysteryX]

Thanks foxyshadis!

What if the source is in 3 planes? Perhaps there's a simple function somewhere to merge/split the 3 planes?

I see there are lots of functions in that file dealing with float. Does it convert to 32-bit float or to half-float?

if the starting point is memcpy'ing two bytes at a time perhaps taking some time to explain function calls and their overhead would be in order before covering more advanced material

matt 7:6
It's not my mother language :) Nobody told me memcpy was an expensive word! and I was too busy programming against DirectX9 to care about such basics. That's why you guys are here: to tell me what I missed along the road!

[MysteryX]

Is there a standard video format that is non-planar YV24? With RGB32, a tweak is needed: the video needs to be shifted up-side-down!

To convert from UINT16 into YV12, would the best approach be to do 3 passes?
1. RGB32: BitBlt into RGB32, reversing the image up-side-down
2. YV24: Split into 3 planes (reversing again)
3. YV12: Call ConvertToYV12()

In which case, your code would do Pass #1 (opposite conversion), and I would need the reverse of that, and some code for Pass #2.

When working with BYTE data, a regular BitBlt would work for Pass #1... if I didn't have to shift it up-side-down.

[MysteryX]

Essentially, the assembly code is to process 8 pixels at once (or even 16 pixels at once). The rest of the code around it, like how to loop through lines, is normal. If I do several separate passes, then I waste memory by having to store the whole frame after each pass.

What might be ideal is to have functions that perform the work on a line, or even on a 8/16 pixels buffer, and then I can call various assembly functions depending on what I need to do for that pass of processing.

I can take a look at redesigning the code around it, and when I'm ready, post the signatures of the functions that would need to do the core processing.

[jpsdr]

@ndjamena
The only remark i would have, is that your code is not endian free (in case you wanted to make a realy pure generic C code).
... And make me realise a mistake in what i wrote, now corrected.

[ndjamena]

I was just playing around. It kind of looks like it converts to 16 bit and THEN shifts it, I have no idea how this works but isn't that two operations or can it be done in one?

I assumed the point WAS to multiply by 256, which will give 8 bits of empty precision.

I have no idea, but if I don't play around with it I never will.

[MysteryX]

foxyshadis, I have edited your function for my needs. I would need the assembly code to merge 3 planes into a 8-byte buffer. Could you help me with that?

Converting from RGB32 instead of from YV24 would be an easy modification from there.

EDIT: This code... each source pixel is 4 bytes. Does this process 8 bytes at once (2 pixels), or 8 pixels at once? To process 8 pixels at once, each source plane must be %8, and the destination pitch must be %64. I can specify the align when creating my destination frame.

// restrictions: src_stride MUST BE a multiple of 8, dst_stride MUST BE a multiple of 64 and 8x src_stride (x4 planes, x2 pixel size)
void ConvertToFloat::bitblt_i8_to_i16_sse2(const uint8_t* srcY, const uint8_t* srcU, const uint8_t* srcV, int srcPitch, uint16_t* dst, int dstPitch, int height)
{
assert(srcPitch % 8 == 0);
assert(dstPitch % 64 == 0);
assert(dstPitch / srcPitch == 8);

const __m128i zero = _mm_setzero_si128();
const __m128i val_ma = _mm_set1_epi16(0);
const __m128i mask_lsb = _mm_set1_epi16(0x00FF);

uint8_t convBuffer[32];

for (int y = 0; y < height; ++y) {
for (int x = 0; x < srcPitch; x += 8) {
// 1. Merge 8 pixels from 3 planes into buffer.


// 2. Convert 8 pixels from buffer into dest
__m128i val = load_8_16l(convBuffer, zero);
val = _mm_slli_epi16(val, 8);
store_8_16l(dst, val, mask_lsb);

// 3. Move to the next stride
srcY += 8;
srcU += 8;
srcV += 8;
dst += 64;
}
}
}

[MysteryX]

OK, this code "should" work. It could be further optimized by copying 8 pixels at once from the 3 planes, but I think it's good enough.

However, the result is a blank green screen; there's no video data whatsoever.

What needs to be tweaked?

// restrictions: src_stride MUST BE a multiple of 8, dst_stride MUST BE a multiple of 64 and 8x src_stride (x4 planes, x2 pixel size)
void ConvertToFloat::bitblt_i8_to_i16_sse2(const uint8_t* srcY, const uint8_t* srcU, const uint8_t* srcV, int srcPitch, uint16_t* dst, int dstPitch, int height)
{
assert(srcPitch % 2 == 0);
assert(dstPitch % 16 == 0);
assert(dstPitch / srcPitch == 8);

const __m128i zero = _mm_setzero_si128();
const __m128i val_ma = _mm_set1_epi16(0);
const __m128i mask_lsb = _mm_set1_epi16(0x00FF);

uint8_t convBuffer[8];
convBuffer[3] = 255;
convBuffer[7] = 255;

for (int y = 0; y < height; ++y) {
for (int x = 0; x < srcPitch; x += 2) {
// 1. Merge 2 pixels from 3 planes into buffer.
convBuffer[0] = srcY[0];
convBuffer[1] = srcU[0];
convBuffer[2] = srcV[0];
convBuffer[4] = srcY[1];
convBuffer[5] = srcU[1];
convBuffer[6] = srcV[1];

// 2. Convert 8 bytes from UINT8 buffer into UINT16 dest
__m128i val = load_8_16l(convBuffer, zero);
val = _mm_slli_epi16(val, 8);
store_8_16l(dst, val, mask_lsb);

// 3. Move to the next stride
srcY += 2;
srcU += 2;
srcV += 2;
dst += 8; // dst is uint16_t so this moves 16 bytes
}
}
}

[MysteryX]

Anyone can help with this? Once I have a bit of assembly optimization in the conversion, I think AviSynthShader would be pretty much ready for version 1.0

[MysteryX]

The code changed a little bit which would make the assembly code a bit more complex. We must add 128 when converting to avoid darkening.

outY[0] = (pOut[0] <= UINT16_MAX - 128 ? pOut[0] + 128 : pOut[0]) >> 8;

[foxyshadis]

Truncation will do that, yeah. Rounding at least keeps the bias to zero; you could do better with a pattern dither, but at least this works.

Also:
uint32_t overflowed = (uint32_t)pOut[0] + 128
outY[0] = -(overflowed>>16) | (uint16_t)overflowed;

Branchless is best in the pixel loop (but profile just in case). sse2 has native saturated addition that would make that a single instruction. Kinda too buzzed to do it right now.

[MysteryX]

mmm sqrt(9801) proposed an interesting solution.

If I multiply the source by 257 instead of 256, then the max value is 255*257=65535. Evenly distributed when converted in, and then evenly distributed when converting back.

That should work... *except* that then I couldn't convert in with a pixel shift. In your opinion, which approach would be best?

EDIT: No, multiplying by 257 and then dividing by 256 would cause a subtle shift in colors.

[MysteryX]

Oh my... that tweak is in the conversion back. That one we haven't yet written in assembly.

So let's first get the convert *to* working before writing the conversion *from*, although the logic will be very similar.

[MysteryX]

uint32_t overflowed = (uint32_t)pOut[0] + 128
outY[0] = -(overflowed>>16) | (uint16_t)overflowed;

That doesn't work


const uint16_t* pSrc = (uint16_t*)src;
uint32_t overflowed = (uint32_t)pSrc[0] + 128;
outY[0] = -(overflowed >> 16) | (uint16_t)overflowed;
overflowed = (uint32_t)pSrc[1] + 128;
outU[0] = -(overflowed >> 16) | (uint16_t)overflowed;
overflowed = (uint32_t)pSrc[2] + 128;
outV[0] = -(overflowed >> 16) | (uint16_t)overflowed;