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

[Jose Fonseca]

----- Original Message -----
> 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.

Precisely. I had no intention to single you out, but simply put a brake on this trend. I always warned against it in the past, but it was my mistake -- I should've drawn the line much sooner.

> 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.

The helpers functions per-se are fine by me if they do something useful, or there's the plan to make them do something useful in the future.  It's the invoking util_format_description() on every single one of them that I don't want.  At this pace we will be invoking it thousands of times per state change unnecessarily.

> 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.

Thanks. I was not really looking for a discussion on coding style either and my remarks were not aimed at you in particular. Sorry if that's what seemed like.

Jose