On Fri, 2012-07-13 at 08:20 +0200, Gwenole Beauchesne wrote:
> Hi,
>
> 2012/7/13 Zhao Halley <halley.z...@intel.com>:
>
> > + case VA_FOURCC('A','Y','U','V'):
>
> Drop all references to AYUV, that's not is this patchset. :)
>
> + Add RGBX/BGRX image formats.
Currently only subpicture supports RGBA/BGRA formats. If you want to add
new image formats RGBX/BGRX, could you also provide shaders for
RGBX/BGRX<-->RGBX/BGRX so that vaGetPutImage()/vaPutImage() can work for
the same formats ? BTW it would be better to add support for new image
formats and new surface formats in another patch.
> > + case VA_FOURCC('R', 'G', 'B', 'A'):
> > + case VA_FOURCC('R', 'G', 'B', 'X'):
> > + case VA_FOURCC('B', 'G', 'R', 'A'):
> > + case VA_FOURCC('B', 'G', 'R', 'X'):
> > + obj_surface->y_cb_offset = 0;
> > + obj_surface->y_cr_offset = 0;
> > + obj_surface->cb_cr_width = obj_surface->orig_width ;
> > + obj_surface->cb_cr_height = obj_surface->orig_height;
> > + obj_surface->cb_cr_pitch = obj_surface->width * 4;
>
> This doesn't look right.
>
> > @@ -2542,6 +2591,13 @@ get_sampling_from_fourcc(unsigned int fourcc)
> > case VA_FOURCC('Y', 'U', 'Y', '2'):
> > surface_sampling = SUBSAMPLE_YUV422H;
> > break;
> > + case VA_FOURCC('A','Y','U','V'):
> > + case VA_FOURCC('R','G','B','A'):
> > + case VA_FOURCC('R','G','B','X'):
> > + case VA_FOURCC('B','G','R','A'):
> > + case VA_FOURCC('B','G','R','X'):
> > + surface_sampling = SUBSAMPLE_YUV444;
> > + break;
> > default:
> > break;
> > }
>
> Same here.
>
> > @@ -3642,7 +3698,12 @@ i965_GetSurfaceAttributes(
> > if (attrib_list[i].value.value.i != VA_FOURCC('N',
> > 'V', '1', '2') &&
> > attrib_list[i].value.value.i != VA_FOURCC('I',
> > '4', '2', '0') &&
> > attrib_list[i].value.value.i != VA_FOURCC('Y',
> > 'V', '1', '2') &&
> > - attrib_list[i].value.value.i != VA_FOURCC('Y',
> > 'U', 'Y', '2')) {
> > + attrib_list[i].value.value.i != VA_FOURCC('Y',
> > 'U', 'Y', '2') &&
> > + attrib_list[i].value.value.i != VA_FOURCC('A',
> > 'Y', 'U', 'V') &&
> > + attrib_list[i].value.value.i != VA_FOURCC('B',
> > 'G', 'R', 'A') &&
> > + attrib_list[i].value.value.i != VA_FOURCC('B',
> > 'G', 'R', 'X') &&
> > + attrib_list[i].value.value.i != VA_FOURCC('R',
> > 'G', 'B', 'X') &&
> > + attrib_list[i].value.value.i != VA_FOURCC('R',
> > 'G', 'B', 'A')) {
> > attrib_list[i].value.value.i = 0;
> > attrib_list[i].flags &=
> > ~VA_SURFACE_ATTRIB_SETTABLE;
> > }
>
> You now see why I recommended to use a switch/case in the first place...
>
> > diff --git a/src/i965_post_processing.h b/src/i965_post_processing.h
> > index 21e525c..64229e8 100755
> > --- a/src/i965_post_processing.h
> > +++ b/src/i965_post_processing.h
> > @@ -51,9 +51,10 @@ enum
> > PP_PL3_LOAD_SAVE_PA,
> > PP_PA_LOAD_SAVE_NV12,
> > PP_PA_LOAD_SAVE_PL3,
> > + PP_RGBX_LOAD_SAVE_NV12,
> > };
> >
> > -#define NUM_PP_MODULES 13
> > +#define NUM_PP_MODULES 14
>
> I wonder why NUM_PP_MODULES is not in the enum. That'd prevent you
> from increasing that value all the time. Please produce another patch
> with this change.
>
> Thanks,
> Gwenole.
> _______________________________________________
> Libva mailing list
> Libva@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libva
_______________________________________________
Libva mailing list
Libva@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libva
