[Mesa-dev] [PATCH 5/9] vl/video_buffer: add YUYV and UYVY support

[Christian König]

On 08.03.2012 22:36, Jose Fonseca wrote:
>
> ----- Original Message -----
>> This gets xine working with VDPAU.
>>
>> v2: some minor bugfixes.
>> v3: create the resource with the subsampled
>>      format to avoid tilling problems
>>
>> Signed-off-by: Christian König<deathsimple at vodafone.de>
> [...]
>> @@ -269,18 +307,21 @@ vl_video_buffer_sampler_view_components(struct
>> pipe_video_buffer *buffer)
>>
>>      pipe = buf->base.context;
>>
>> +   sampler_format = vl_video_buffer_formats(pipe->screen,
>> buf->base.buffer_format);
>>      plane_order =
>>      vl_video_buffer_plane_order(buf->base.buffer_format);
>>
>>      for (component = 0, i = 0; i<  buf->num_planes; ++i ) {
>>         struct pipe_resource *res = buf->resources[plane_order[i]];
>>         unsigned nr_components =
>>         util_format_get_nr_components(res->format);
>> +      if (util_format_is_subsampled(res->format))
> Please, no more of this sort of helpers functions such as  util_format_get_nr_components() and util_format_is_subsampled() which do nothing more than invoking util_format_description() and read a member variable.
>
> Please invoke util_format_description() only once here and then access any info as you please.
>
> > From now on I'm going to refuse any more u_format helpers that take pipe_format as arguments -- they appear to simplify things but in truth they just promote inefficient and redundant code. External functions that want to reason about formats should invoke util_format_description() or receive the format description as parameter.
Well, that's how the code looked originally, and I have no problem 
changing it back to that state. But then we should clean up all the 
other functions as well, cause my intention was to follow what I thought 
was the general usage style of util_format_* informations.

On the other hand what is so bad about single line INLINED accessor 
functions (apart from the obvious: That every problem in computer since 
can be solved with another layer of indirection)?

Assuming  at some point in the future we want to add an 
UTIL_FORMAT_LAYOUT_PLANAR, then we would need to check every access of 
util_format_description->layout if adding that enumeration value doesn't 
break its usecase. With accessor functions and only indirect using 
util_format_description information we won't have that problem, cause 
every usecase is handled by that functions and not directly in the code 
using that component.

But as you already stated, that approach has the possibility of creating 
lots of accessor functions and bloating the code quite a bit.

Anyway I'm going to change it back to the original state and committing 
the result, since I really have better things todo than starting a 
discussion about coding style.

Christian.