Enable GLX/EGL backend switching while run HW video decode with libva.

Enable GLX/EGL backend switching while run HW video decode with libva.

    Some IA platforms do not rely on GLX but EGL as graphic texture backend
    within HW video decoding acceleration by libva (e.g. va_putsurface).

    This will enhance Chromeos's VAVDA in IA graphic backend which rely
    on the egl not the former default glx. The developer need specific TAG
    '-Dvavda_vagl_egl=1' for switching to egl backend while running gyp,
    the default glx backend keep as before if do not specific the TAG.

    Devices build-in new X86 libva with egl as graphic backend will need this.

    BUG=NONE
    Review URL: https://codereview.chromium.org/43283002

[qing.zhang, 2013-10-25 06:20:11]

Hi, xhwang, Ami Fischman and piman,

I am be very appreciate if you would have time to look at my changes. 

Does it meaningful to have a solution with EGL backend for HW video decode
acceleration with VA for some X86 platforms.

[xhwang, 2013-10-25 16:14:29]

I am not familiar with this area. I'll defer to fischman to review this CL.

https://codereview.chromium.org/43283002/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/43283002/diff/1/AUTHORS#newcode309
AUTHORS:309: Zhand Qing <qing.zhang@intel.com>
s/Zhand/Zhang

Just FYI, most people use "FirstName LastName" format.

[piman, 2013-10-25 20:40:00]

Is there some abstraction we could add to avoid ifdefs everywhere?

[Ami GONE FROM CHROMIUM, 2013-10-25 20:43:39]

On Fri, Oct 25, 2013 at 1:40 PM, <piman@chromium.org> wrote:

> Is there some abstraction we could add to avoid ifdefs everywhere?
>

+1
Also, do we have any of this HW in any chromium waterfall?  I'm loathe to
add all this conditional code everywhere if it'll have zero bot coverage.


>
>
https://codereview.chromium.**org/43283002/<https://codereview.chromium.org/4...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[qing.zhang, 2013-10-26 15:10:37]

On 2013/10/25 20:43:39, Ami Fischman wrote:
> On Fri, Oct 25, 2013 at 1:40 PM, <mailto:piman@chromium.org> wrote:
> 
> > Is there some abstraction we could add to avoid ifdefs everywhere?
> >
> 
> +1
> Also, do we have any of this HW in any chromium waterfall?  I'm loathe to
> add all this conditional code everywhere if it'll have zero bot coverage.
> 
> 

Yes. Agree both of piman@'s and fischman@'s comments. That's more decent.

I'm worry the incomplete abstraction would bring a giant patch. Also,  
except adding new isolate ".cc" and ".h" files, may i have your suggestions 
or tips for enabling EGL backend with less "ifdef" ?

Thank you very much.

[Ami GONE FROM CHROMIUM, 2013-10-26 19:20:50]

I don't think we have an out-of-the-box solution for you.  The typical way
to address this sort of thing is to extract the differing implementations
into specialized objects that can be swapped in with a single #ifdef, each
of which exposes an API surface that allows the current locations of the
one impl to delegate to them instead.  E.g. how GpuVideoDecodeAccelerator
swaps in platform-specific VDA impls in the first place.

(not sure if I said this before, but) please make sure your CL/HW combo
passes video_decode_accelerator_unittest.


On Sat, Oct 26, 2013 at 8:10 AM, <qing.zhang@intel.com> wrote:

> On 2013/10/25 20:43:39, Ami Fischman wrote:
>
>  On Fri, Oct 25, 2013 at 1:40 PM, <mailto:piman@chromium.org> wrote:
>>
>
>  > Is there some abstraction we could add to avoid ifdefs everywhere?
>> >
>>
>
>  +1
>> Also, do we have any of this HW in any chromium waterfall?  I'm loathe to
>> add all this conditional code everywhere if it'll have zero bot coverage.
>>
>
>
>
> Yes. Agree both of piman@'s and fischman@'s comments. That's more decent.
>
> I'm worry the incomplete abstraction would bring a giant patch. Also,
> except adding new isolate ".cc" and ".h" files, may i have your suggestions
> or tips for enabling EGL backend with less "ifdef" ?
>
> Thank you very much.
>
>
>
https://codereview.chromium.**org/43283002/<https://codereview.chromium.org/4...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[qing.zhang, 2013-10-27 11:28:52]

>On 2013/10/26 19:20:50, Ami Fischman wrote:

Ami Fischman@ Your comments and tips are very helpful, thanks a lot.

I will refine it soon.

[qing.zhang, 2013-10-28 06:01:40]

Hi, Fischman and piman,

This extension VA_EGL patch had been refined and verified. 
Appreciate to your times and patient to review it again. 

Thanks a lot.

[Ami GONE FROM CHROMIUM, 2013-10-28 19:50:14]

I don't think copy/pasting >1k LOC is an acceptable solution to the problem
of pervasive #if's.
What I was hoping for in my previous comment was that you'd be able to
extract a coherent subset of VAVDA that'd encapsulate the functionality
that differs between EGL & GLX impls, phrase that as an interface, and
provide two impls of that interface.  (or templatize VAVDA on the impl, or
use another scheme; the point isn't virtual dispatch but rather that BOTH
the amount of code duplication AND the number of places in the code where
dispatch differs by platform are minimized)


On Sun, Oct 27, 2013 at 11:01 PM, <qing.zhang@intel.com> wrote:

> Hi, Fischman and piman,
>
> This extension VA_EGL patch had been refined and verified.
> Appreciate to your times and patient to review it again.
>
> Thanks a lot.
>
>
https://codereview.chromium.**org/43283002/<https://codereview.chromium.org/4...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[qing.zhang, 2013-10-29 07:26:19]

Hi, Ami Fischman and piman,

Thank you very much to help me review.
How about this one? Any incorrect, let me know freely.

Regards

[Ami GONE FROM CHROMIUM, 2013-10-29 15:44:27]

This is better than the copy/paste approach, but still has a bunch of
per-platform conditionals.  Basically this replaces PS#1's #if's with if's.  I
still suspect it should be possible to abstract out the EGL-vs-GLX bits from
VAVDA & TFPPicture so that the per-platform conditionality is constrained to
just a couple of places.

I still have serious concerns about the plan here, though:
1) Are there plans to add this HW to any chromium waterfall?  Any code not
exercised by bots is likely to bitrot constantly.
2) This CL enables VAVDA for non-chromeos platforms; I suspect you did this for
linux specifically, but actually you end up catching a lot more than just
linux&chromeos by saying !win && !mac (e.g. android, ios, etc).  VAVDA should
continue to be restricted to chromeos==1 in gyp.  (for dev & testing, note that
chromeos=1 builds can be run on linux; e.g.
https://code.google.com/p/chromium/wiki/LinuxHWVideoDecode)

Separate from the above, some style issues:
a) IWBN to avoid having always-NULL/false variables floating around (i.e. the
egl_* vars in a GLX build, etc).  
b) glx_backend_ is unnecessary given the way it's used (you can always test the
relevant members without testing glx_backend_ in addition)
c) s/}else/} else/g, and indentation is off elsewhere; clang-format is your
friend.

[qing.zhang, 2013-10-30 14:21:04]

Very helpful comments, thanks you, Fischman.

"I still have serious concerns about the plan here, though"
--> You are right, I just want to enhance the chromeos Vaapi
solution in new IA graphic which only support egl backend.
So, at the beginning i try to make it to be general enabling
which looks not very suitable after your inputs. Now, clearly 
to know, that should be covered into the Chromeos's build. 
Refined show as <Set#5>.

"Separate from the above, some style issues"
--> I'v try to fix some of it as much as possible and do some abstraction.
But i'm not sure whether it be accepted even i know there are several better
schemes and i just use one of basic of them. 

Again, thanks for your patient and helps for this CL.
Please let me know your options freely.


On 2013/10/29 15:44:27, Ami Fischman wrote:
> This is better than the copy/paste approach, but still has a bunch of
> per-platform conditionals.  Basically this replaces PS#1's #if's with if's.  I
> still suspect it should be possible to abstract out the EGL-vs-GLX bits from
> VAVDA & TFPPicture so that the per-platform conditionality is constrained to
> just a couple of places.
> 
> I still have serious concerns about the plan here, though:
> 1) Are there plans to add this HW to any chromium waterfall?  Any code not
> exercised by bots is likely to bitrot constantly.
> 2) This CL enables VAVDA for non-chromeos platforms; I suspect you did this
for
> linux specifically, but actually you end up catching a lot more than just
> linux&chromeos by saying !win && !mac (e.g. android, ios, etc).  VAVDA should
> continue to be restricted to chromeos==1 in gyp.  (for dev & testing, note
that
> chromeos=1 builds can be run on linux; e.g.
> https://code.google.com/p/chromium/wiki/LinuxHWVideoDecode)
> 
> Separate from the above, some style issues:
> a) IWBN to avoid having always-NULL/false variables floating around (i.e. the
> egl_* vars in a GLX build, etc).  
> b) glx_backend_ is unnecessary given the way it's used (you can always test
the
> relevant members without testing glx_backend_ in addition)
> c) s/}else/} else/g, and indentation is off elsewhere; clang-format is your
> friend.

[qing.zhang, 2013-10-30 14:25:00]

Correct: 
Please let me know your opinions (not 'options') freely.

[Ami GONE FROM CHROMIUM, 2013-10-30 20:07:55]

PS#5 addressed all my concerns except:

1) This replaces PS#1's #if's with if's.  I suspect it should be possible to
abstract out the EGL-vs-GLX bits from VAVDA & TFPPicture so that the
per-platform conditionality is constrained to just a couple of places.
2) Are there plans to add this HW to any chromium waterfall?  Any code not
exercised by bots is likely to bitrot constantly.

[Ami GONE FROM CHROMIUM, 2013-10-30 20:08:05]

https://codereview.chromium.org/43283002/diff/250001/content/content_common.gypi
File content/content_common.gypi (right):

https://codereview.chromium.org/43283002/diff/250001/content/content_common.g...
content/content_common.gypi:526: 'vavda_vagl_egl%': '<(vavda_vagl_egl)',
Is the indirection really necessary?  (I think it is not).

[qing.zhang, 2013-10-31 11:49:21]

On 2013/10/30 20:07:55, Ami Fischman wrote:
> PS#5 addressed all my concerns except:
> 
> 1) This replaces PS#1's #if's with if's.  I suspect it should be possible to
> abstract out the EGL-vs-GLX bits from VAVDA & TFPPicture so that the
> per-platform conditionality is constrained to just a couple of places.
> 2) Are there plans to add this HW to any chromium waterfall?  Any code not
> exercised by bots is likely to bitrot constantly.

Thank you, Fischmen.

"1) This .." 
--> I will keep refining aline with your comments if you feel this CL is the
right way to go. 
 
"2) Are .."
--> I've dry run with trybot please refer to
"https://codereview.chromium.org/54143002/"
So, does it make sense ?

[qing.zhang, 2013-10-31 11:50:46]

On 2013/10/30 20:08:05, Ami Fischman wrote:
>
https://codereview.chromium.org/43283002/diff/250001/content/content_common.gypi
> File content/content_common.gypi (right):
> 
>
https://codereview.chromium.org/43283002/diff/250001/content/content_common.g...
> content/content_common.gypi:526: 'vavda_vagl_egl%': '<(vavda_vagl_egl)',
> Is the indirection really necessary?  (I think it is not).

Yes, you are right. 
I will modify it in the next PS#