[FFmpeg-devel] [PATCH] libi264: Add Hardware Accelerated H.264 Encoder based on libVA

[James Almer]

On 12/31/2015 10:11 PM, compn wrote:
> On Thu, 31 Dec 2015 22:07:56 +0100
> Michael Niedermayer <michael at niedermayer.cc> wrote:
> 
>> On Thu, Dec 31, 2015 at 10:35:47PM +0500, hamza at mayartech.com wrote:
>>> From: Bryan Christ <bryan.christ at mediafire.com>
>>>    --enable-libx264         enable H.264 encoding via x264 [no]
>>> +  --enable-libi264         enable H.264 encoding via Intel's libva
> 
> is there a difference between intel libva and other libva ? otherwise
> can it just be "via libva" ?
> 
>> [...]
>>> +    for (row = 0; row < frame->height/2; row++) {
>>> +        unsigned char *U_row = U_start + row * U_pitch;
>>> +        unsigned char *u_ptr = NULL, *v_ptr=NULL;
>>> +//      int j;
>>> +        int j, N, Nmod;
>>> +        switch (surface_image.format.fourcc) {
>>> +        case VA_FOURCC_NV12:
>>> +            u_ptr = frame->data[1] + row * frame->linesize[1];
>>> +            v_ptr = frame->data[2] + row * frame->linesize[2];
>>> +
>>> +
>>> +                        Nmod = (frame->width/2) & 7; // mod 8
>>> +                        N    = (frame->width/2) - Nmod;
>>> +                        __asm__(
>>> +                                "movq      %0,      %%rax  \n\t"
>>> +                                "movq      %1,      %%rbx  \n\t"
>>> +                                "movq      %2,      %%rcx  \n\t"
>>> +                                "movq      %3,      %%rdx  \n\t"
>>> +                                "asm_loop:                 \n\t"
>>> +                                "movq      (%%rax), %%xmm0 \n\t"
>>> +                                "movq      (%%rbx), %%xmm1 \n\t"
>>> +                                "punpcklbw %%xmm1,  %%xmm0 \n\t"
>>> +                                "movdqu    %%xmm0,  (%%rcx)\n\t"
>>> +                                "addq      $0x8,    %%rax  \n\t"
>>> +                                "addq      $0x8,    %%rbx  \n\t"
>>> +                                "addq      $0x10,   %%rcx  \n\t"
>>> +                                "cmp       %%rcx,   %%rdx  \n\t"
>>> +                                "jnz       asm_loop"
>>> +                                :
>>> +                                : "r"(u_ptr), "r"(v_ptr),
>>> "r"(U_row),
>>> +                                  "r" (U_row+2*N)
>>> +                                : "rax", "rbx", "rcx", "rdx",
>>> "xmm0", "xmm1"
>>> +                        );
>>
>> x86* asm belongs in yasm files
>> colorspace convertion belongs to vf_scale / swscale, why is this
>> code here ?
> 
> if i may guess, colorspace conversion within this lib is so that an
> external application can call lavc's i264 encoder without converting
> nv12 itself.
> 
> 
>> also FFmpeg supports many platforms, not just x86 based ones
>> asm should be behind appropriate ARCH_* & cpuflags checks
> 
> yes but i think this patch is for specifically x86 intel libva
> 
> although right now i'm guessing there is also x64 intel libva?
> 
> rephrased, this is a feature just for x86 libva h264 encoding on intel
> chipsets. 
> 
> would that be ok to commit as-is? and then add support for
> other platforms later? i ask because sometimes it is monumental task to

No, it can't. For starters, arch specific code can't be outside the relevant
arch specific folder, and we've been trying to get rid of x86 inline asm
outside of inlined functions like those from lavu's intreadwrite.h for a while
now.

Others may also have comments about the rest of the patch.