Code review comment for lp:~andreas-pokorny/mir/add-pixel-format-to-display-configuration

> 466 +MirPixelFormat select_format(MirPixelFormat format,
> std::vector<MirPixelFormat> const& formats)
> 467 +{
> 468 + if (formats.empty())
> 469 + return mir_pixel_format_invalid;
> 470 +
> 471 + if (!mg::contains_alpha(format))
> 472 + return format;
> 473 +
> 474 + auto opaque_pos = std::find_if_not(formats.begin(), formats.end(),
> mg::contains_alpha);
> 475 +
> 476 + if (opaque_pos == formats.end())
> 477 + return format;
> 478 +
> 479 + return *opaque_pos;
> 480 +}
>
> From the prototype I would expect that select_format() would return a format
> that exists in formats (or possibly fail).
>
> What is the intent of this function? The logic seems weird - is it right?

The idea was that default display configuration policy should try to select an opaque format where possible.
There might be multiple opaque formats, and one might be already configured - so the default policy does not
touch it.
The idea was that a decorated policy selected certain opaque format on purpose.

> Is failure indicated by returning 468 mir_pixel_format_invalid as in ll468-9?
> (If failure is an option I'd expect an exception, if not a "sensible"
> default.)

Yes, that is failure and in some graphics platforms that yields an exception (mesa, nested).
Here I was not sure how the class should behave, since all but nested ignore the value of the
configured output format.

> If format doesn't contain an alpha component it will return format regardless
> of whether format is in formats (unless formats is empty per check on l468).

Yes thats an error case that I did not handle. (It is handled in nested and mesa platform though - with an exception)

> If format does contain alpha as then it it actually uses the contents of
> formats to decide which format to return: It either returns an entry in
> formats (as I initially expected - although it doesn't prioritize what I read
> as the requested format) or it returns format where I expected a failure.

Hm if there is no opaque format in the vector it takes the preconfigured one - blindly assuming that it is part of the vector. Which would then be a format with alpha channel.

> So the return is one of mir_pixel_format_invalid, format, or the first alpha
> format from formats.

no the first non-alpha format - it uses find_if_not