Merge lp:~andreas-pokorny/mir/add-pixel-format-to-display-configuration into lp:mir

[Andreas Pokorny]

Adds a mir pixel format parameter to DisplayConfiguration::configure_output

So within DisplayConfigurationPolicy the format can be specified for each output.
The pixel format parameter is still ignored by android and also mesa to some degree,
which only validates it. It is supported by nested display. The default configuration
policy implementation tries to select an opaque format though.

Adds a mir pixel format parameter to DisplayConfiguration::configure_output

So within DisplayConfigurationPolicy the format can be specified for each output.
The pixel format parameter is still ignored by android and also mesa to some degree,
which only validates it. It is supported by nested display. The default configuration
policy implementation tries to select an opaque format though.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1307
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/596/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/539
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/535
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/143
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/322/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/325
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/325/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/143
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/143/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/170
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2658

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/596/rebuild

review: Needs Fixing (continuous-integration)

[Robert Carr (robertcarr)]

LGTM. A few small comments:

1. It's a shame that all these updates to configure_output are making the diff noisy. It's not entirely clear to me why the PixelFormat needs to go through the configure_output call besides the deficiencies of the interface (no other way from frontend to the nested surfaces). Conceptually it doesn't seem to match. You aren't creating that here though.

2.
"+using namespace mir::graphics;"
253 +namespace mesa
etc...

Unless there has been a recent change Mir style is not to do this and use the aliases. Maybe we should be open to changing this. I'm not fond of using namespace, but could be happy to enclose implementations in namespace scopes in C++ files.

3. This is another thing that you aren't causing, as there seems to be a lot of inconsistency in this area of the code anyway...there are some indentation "errors" though

+ virtual void configure_output(DisplayConfigurationOutputId id, bool used, geometry::Point top_left,
+ size_t mode_index, MirPixelFormat format, MirPowerMode power_mode) = 0;

Typically mir style is instead:
+ virtual void configure_output(DisplayConfigurationOutputId id, bool used, geometry::Point top_left,
+ size_t mode_index, MirPixelFormat format, MirPowerMode power_mode) = 0;

That is to say just follow the 4 space indentation rather than align. Obviously this code isn't respecting that to start though, and I have a feeling it might be my fault so no blocking there ;)

I'll approve but can someone else comment on the namespacing before we top approve?

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1308
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/598/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/541
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/537
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/145
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/324/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/327
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/327/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/145
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/145/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/172
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2665

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/598/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1309
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/601/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/545
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/541
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/149
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/327
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/327/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/330
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/330/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/149
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/149/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/176
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2672

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/601/rebuild

review: Approve (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

1) My first attempt introduced a separate interface that would only serve the purpose of configuring the pixel format iirc. That one got reject in favour of adding a parameter to configure_output

2) wrt: +using mir::graphics it was mg = mir::graphics initially and duflu indicated that we should avoid that style, and suggested "using". In the other file that I touched after that review I changed it to namespace mir { namespace ... { } } as a better compromise. We should discuss that topic.

3) I am still playing around with getting my vim cinoptions into the right shape. I thought I saw that alignment of the second line parameters in the mir style guide. http://unity.ubuntu.com/mir/cppguide/index.html?showone=Function_Declarations_and_Definitions#Function_Declarations_and_Definitions
But then again I was also told that those examples are wrong :)

[Daniel van Vugt (vanvugt)]

Re:
    namespace mg=mir::graphics;
    mg::Something
Namespace aliases are cryptic in the least. We should aim to do better. I suggest a good alternative/compromise is:
    using namespace mir;
    graphics::Something
Although if it's the only "Something" used by the source file then it's even better to reduce syntactic coupling by:
    using mir::graphics::Something;
    Something
I also accept "using namespace mir::graphics" :)

[Daniel van Vugt (vanvugt)]

(1) I agree in real display systems (non-nested) the pixel format is not something you choose. What you choose is a colour depth (which may or may not fully dictate the pixel format), and from that you can query what the physical pixel format is. That's if you even need it at all. In the case of OpenGL for example, you never need to know the physical display pixel format. I'm wondering if should make it more obvious that the MirPixelFormat parameter is only meaningful to nested displays?

My key concern is that although two graphics systems might look similar, one may only support RGB and the next might only support BGR. And a third might only support 16-bit colour (like i8xx GPUs). So specifying a desired pixel format is very dangerous to portability _if_ you require that the format be honored. It's not yet clear to me if we are using it in an unsafe way though.

(2) Namespaces: See previous comment

review: Needs Information

[Andreas Pokorny (andreas-pokorny)]

> (1) I agree in real display systems (non-nested) the pixel format is not
> something you choose. What you choose is a colour depth (which may or may not
> fully dictate the pixel format), and from that you can query what the physical
> pixel format is. That's if you even need it at all. In the case of OpenGL for
> example, you never need to know the physical display pixel format. I'm
> wondering if should make it more obvious that the MirPixelFormat parameter is
> only meaningful to nested displays?
>
> My key concern is that although two graphics systems might look similar, one
> may only support RGB and the next might only support BGR. And a third might
> only support 16-bit colour (like i8xx GPUs). So specifying a desired pixel
> format is very dangerous to portability _if_ you require that the format be
> honored. It's not yet clear to me if we are using it in an unsafe way though.

I think neither for nested nor for the real display outputs specifying a pixel format makes
sense. What is shown on the display changes dynamically based on the content provided.
So if there is content provided by a client that can be scanned out directly the
"current pixel format" at the output as an up-front configuration makes no sense.
If we can somehow leverage more hardware composting support, we might have opaque and
 non opaque RGB buffers and YUV buffers at the same time - and no notion of "current pixel format"
on an output.

As soon as we do that (=hwc) for real displays we will also do that for the nested case.
Then we should also rework NestedDisplay and NestedOutput. Those classes should only create
buffers when the nested shell request buffers for rendering.

But right now the graphical structure (as i understand them) is:
 * there is a mir server that creates a frame buffer
 * the frame buffer contains the rendering result of the shell that draws all its visible clients
and that recursively for all levels of nested servers. Bypass is just an exception through the system.

With the hwc support we discussed in December it may become:
 * there is a mir server
 * the shell decides which set of buffers define the rendering result of the server
and that recursively for all levels of nested servers. Bypass does not exist as a special case.

At that point the "current pixel format" should no longer be part of the Output Configuration.

> (2) Namespaces: See previous comment

What is bad about to opening namespace scope inside source files of the class to be implemented?

[Daniel van Vugt (vanvugt)]

(1) OK I don't care to argue about pixel format passing any more. If nesting needs it right now, then leave it. But I'd like to think we will eventually revisit it for a nicer solution.

(2) Nothing is bad about "using namespace" in most cpp files. In fact, it's very good to reduce the syntactic coupling and improve readability. There are some cases where it could be dangerous, but they're relatively rare.

[Daniel van Vugt (vanvugt)]

review: Abstain

[Daniel van Vugt (vanvugt)]

(1) I know I said I would drop the issue, but now I notice this, my original concerns seem justified. We all know that EGL won't and can't honour the exact format. But passing it as a parameter could give easily the reader a false impression that we expect the given pixel format to be honoured.
680 +EGLConfig mgn::detail::EGLDisplayHandle::choose_windowed_es_config(MirPixelFormat format) const
681 {
682 + EGLint const nested_egl_config_attribs[] =
683 + {
684 + EGL_SURFACE_TYPE, EGL_WINDOW_BIT,
685 + EGL_RED_SIZE, mg::red_channel_depth(format),
686 + EGL_GREEN_SIZE, mg::green_channel_depth(format),
687 + EGL_BLUE_SIZE, mg::blue_channel_depth(format),
688 + EGL_ALPHA_SIZE, mg::alpha_channel_depth(format),
689 + EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
690 + EGL_NONE
691 + };

(2) Back on the subject of namespaces... Although I do endorse "using namespace", introducing it in existing source files can and does inflate the diff unnecessarily (like in real_kms_display_configuration.cpp). I'm not saying change it back, but just beware in future.

(4) The comment doesn't match the code:
63 /** The index in the 'pixel_format' vector of the current output pixel format. */
64 - size_t current_format_index;
65 + MirPixelFormat current_format;

(5) Unnecessary change in indentation:
88 - std::shared_ptr<DisplayConfigurationPolicy> const& initial_conf_policy) = 0;
89 + std::shared_ptr<DisplayConfigurationPolicy> const& initial_conf_policy) = 0;

(6) This is a client ABI break, so we need to bump MIRCLIENT_ABI and debian/* info:
101 - uint32_t current_output_format;
102 + MirPixelFormat current_output_format;

(7) Don't you mean "mode_index >= modes.size()" ?
521 + if (mode_index > modes.size())
522 + return 0;

(8) Please avoid the keyword "inline". The compiler is a better judge of that. Inlining unnecessarily just leads to binary bloat.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

As this seems to be the MP for an argument about namespaces I'll state my reason for a (mild) preference for introducing short aliases as done by the majority of existing code:

Assume we have a header containing "namespace example { <declaration> }"

Then in an implementation we want to provide a definition.

We can do this many ways:

1. "namespace example { <definition> }"
2. "example::<definition>"
3. "namespace eg = example; eg::<definition>"

What we can't do (as it defines a different entity in global namespace) is:

4. "using namespace example; <definition>"

In the past I've seen projects trip over the "definition is an implicit declaration" trap due to mismatched names in header and implementation. (The trap is that there is no compile-time error as nothing tells the compiler that the declaration and definition are intended to reference the same thing.)

In order to get an early (compile-time) error I prefer style 2 or 3 in these cases. That approach is largely followed in the Mir codebase.

I am aware that we often have a definition in the header (typically a class def) and the equivalent of 4 works for defining member functions. My preference is for a consistent approach over a more complex rule depending upon whether the header has a definition or a declaration or whether we are defining member functions or functions.

[Andreas Pokorny (andreas-pokorny)]

> (1) I know I said I would drop the issue, but now I notice this, my original
> concerns seem justified. We all know that EGL won't and can't honour the exact
> format. But passing it as a parameter could give easily the reader a false
> impression that we expect the given pixel format to be honoured.
> 680 +EGLConfig
> mgn::detail::EGLDisplayHandle::choose_windowed_es_config(MirPixelFormat
> format) const
> 681 {
> 682 + EGLint const nested_egl_config_attribs[] =
> 683 + {
> 684 + EGL_SURFACE_TYPE, EGL_WINDOW_BIT,
> 685 + EGL_RED_SIZE, mg::red_channel_depth(format),
> 686 + EGL_GREEN_SIZE, mg::green_channel_depth(format),
> 687 + EGL_BLUE_SIZE, mg::blue_channel_depth(format),
> 688 + EGL_ALPHA_SIZE, mg::alpha_channel_depth(format),
> 689 + EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
> 690 + EGL_NONE
> 691 + };

What is your suggestion here? Make a cosmetic change and split the pixel format into four components somewhere higher in the call hierarchy? Rework the frame buffer allocation responsibility entirely? But still at some point someone requests a certain format and EGL tried to find one or more that match. The relevant piece here is that EGL_ALPHA_SIZE is zero in the right cases and non-zero in the other cases.

>
> (2) Back on the subject of namespaces... Although I do endorse "using
> namespace", introducing it in existing source files can and does inflate the
> diff unnecessarily (like in real_kms_display_configuration.cpp). I'm not
> saying change it back, but just beware in future.

That was my reaction to your complaint on mg = mir::graphics; + excessive Boy Scouting. I cannot second using using :) in most cases, but I had the impression that we should abandon the acronym-alias style - hence removed it from a file I touched afterwards.

> (4)-(8)

aye - thanks for paying so much attention!

[Daniel van Vugt (vanvugt)]

(1) I don't have a suggestion for something better right now, so won't block on it. Just a concern.

(2) Now you're aware of our varying views, use your own judgement. Namespace style won't usually be a blocker, so long as you're sensible (which you are).

(4)-(8) Need fixing.

(9) You have a prerequisite on a branch which is not up for review ("Work in progress"). This means some of the diff is hidden and not visible on this page. Please ensure prerequisite branches are up for review first.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1310
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/628/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/580/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/576
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/184/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/354
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/354/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/357/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/184/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/628/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1311
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/634/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/591
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/587
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/195
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/360
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/360/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/363
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/363/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/195
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/195/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/216
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2817

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/634/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

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?

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

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

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.

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

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1312
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/637/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/595
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/591
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/199
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/363
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/363/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/366
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/366/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/199
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/199/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/220
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2835

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/637/rebuild

review: Approve (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

> 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

[Daniel van Vugt (vanvugt)]

(6) There's a real danger of external projects compiling successfully with this change, and then crashing with out-of-bounds array accesses:
127 - uint32_t current_output_format;
128 + MirPixelFormat current_output_format;
because we kept the same name and both are int-compatible but have different meanings. To remove the risk of such regressions, I suggest renaming fields; something like "current_format".

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> The idea was that default display configuration policy should try to select an
> opaque format where possible.
...
> 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

I'd find it easier to follow like this:

MirPixelFormat select_opaque_format(MirPixelFormat format, std::vector<MirPixelFormat> const& formats)
{
    auto const format_in_formats = formats.end() != std::find(formats.begin(), formats.end(), format);

    if (!mg::contains_alpha(format) && format_in_formats)
        return format;

    // format is either unavailable or transparent
    auto const first_opaque = std::find_if_not(formats.begin(), formats.end(), mg::contains_alpha);

    if (first_opaque != formats.end())
        return *first_opaque;

    // only tranparent options - allow choice if available
    if (format_in_formats)
        return format;

    if (formats.size())
        return formats.at(0);

    return mir_pixel_format_invalid;
}

review: Abstain

[Andreas Pokorny (andreas-pokorny)]

yes a lot better.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1313
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/648/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/615
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/611
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/219/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/374
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/374/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/377
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/377/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/219
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/219/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/241/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2923

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/648/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

review: Abstain

[Daniel van Vugt (vanvugt)]

(9) Duplicate alias definition:
799 +namespace mgn = mg::nested;
800 namespace mgn = mir::graphics::nested;
But not important.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1314
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/656/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/627
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/623
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/231
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/382
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/382/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/385
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/385/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/231
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/231/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/252
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/2963

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/656/rebuild

review: Approve (continuous-integration)

=== modified file 'debian/control'
--- debian/control	2014-01-10 05:07:43 +0000
+++ debian/control	2014-01-10 10:21:28 +0000
@@ -128,7 +128,7 @@
  .
  Contains header files required to build Mir servers.
 
-Package: libmirclient4
+Package: libmirclient5
 Section: libs
 Architecture: i386 amd64 armhf arm64
 Multi-Arch: same
@@ -146,7 +146,7 @@
 Architecture: i386 amd64 armhf arm64
 Multi-Arch: same
 Pre-Depends: ${misc:Pre-Depends}
-Depends: libmirclient4 (= ${binary:Version}),
+Depends: libmirclient5 (= ${binary:Version}),
          libmirprotobuf-dev (= ${binary:Version}),
          mircommon-dev (= ${binary:Version}),
          ${misc:Depends},
 
=== renamed file 'debian/libmirclient4.install' => 'debian/libmirclient5.install'
--- debian/libmirclient4.install	2013-10-21 17:49:01 +0000
+++ debian/libmirclient5.install	2014-01-10 10:21:28 +0000
@@ -1,1 +1,1 @@
-usr/lib/*/libmirclient.so.4
+usr/lib/*/libmirclient.so.5
 
=== modified file 'examples/demo-shell/window_manager.cpp'
--- examples/demo-shell/window_manager.cpp	2014-01-10 03:23:02 +0000
+++ examples/demo-shell/window_manager.cpp	2014-01-10 10:21:28 +0000
@@ -151,8 +151,9 @@
                     power_mode = mir_power_mode_off;
 
                 conf->configure_output(output.id, output.used,
-                                       output.top_left, 
+                                       output.top_left,
                                        output.current_mode_index,
+                                       output.current_format,
                                        power_mode);
             });
             display_off = !display_off;
 
=== modified file 'examples/server_configuration.cpp'
--- examples/server_configuration.cpp	2013-10-21 10:40:19 +0000
+++ examples/server_configuration.cpp	2014-01-10 10:21:28 +0000
@@ -61,14 +61,16 @@
                     available_outputs_for_card[conf_output.card_id] > 0)
                 {
                     conf.configure_output(conf_output.id, true, geom::Point{max_x, 0},
-                                          preferred_mode_index, mir_power_mode_on);
+                                          preferred_mode_index, conf_output.current_format,
+                                          mir_power_mode_on);
                     max_x += conf_output.modes[preferred_mode_index].size.width.as_int();
                     --available_outputs_for_card[conf_output.card_id];
                 }
                 else
                 {
                     conf.configure_output(conf_output.id, false, conf_output.top_left,
-                                          conf_output.current_mode_index, mir_power_mode_on);
+                                          conf_output.current_mode_index, conf_output.current_format,
+                                          mir_power_mode_on);
                 }
             });
     }
@@ -88,13 +90,15 @@
                 if (!done && conf_output.connected && conf_output.modes.size() > 0)
                 {
                     conf.configure_output(conf_output.id, true, geom::Point{0, 0},
-                                          preferred_mode_index, mir_power_mode_on);
+                                          preferred_mode_index, conf_output.current_format,
+                                          mir_power_mode_on);
                     done = true;
                 }
                 else
                 {
                     conf.configure_output(conf_output.id, false, conf_output.top_left,
-                                          conf_output.current_mode_index, mir_power_mode_on);
+                                          conf_output.current_mode_index, conf_output.current_format,
+                                          mir_power_mode_on);
                 }
             });
     }
 
=== modified file 'include/platform/mir/graphics/display_configuration.h'
--- include/platform/mir/graphics/display_configuration.h	2013-12-18 02:19:19 +0000
+++ include/platform/mir/graphics/display_configuration.h	2014-01-10 10:21:28 +0000
@@ -22,6 +22,7 @@
 #include "mir/int_wrapper.h"
 #include "mir/geometry/size.h"
 #include "mir/geometry/point.h"
+#include "mir/graphics/pixel_format_utils.h"
 #include "mir_toolkit/common.h"
 
 #include <functional>
@@ -103,8 +104,8 @@
     geometry::Point top_left;
     /** The index in the 'modes' vector of the current output mode. */
     size_t current_mode_index;
-    /** The index in the 'pixel_format' vector of the current output pixel format. */
-    size_t current_format_index;
+    /** The current output pixel format. A matching entry should be found in the 'pixel_formats' vector*/
+    MirPixelFormat current_format;
     /** Current power mode **/
     MirPowerMode power_mode;
 };
@@ -135,8 +136,8 @@
     virtual void for_each_output(std::function<void(DisplayConfigurationOutput const&)> f) const = 0;
 
     /** Configures an output. */
-    virtual void configure_output(DisplayConfigurationOutputId id, bool used,
-                                  geometry::Point top_left, size_t mode_index, MirPowerMode power_mode) = 0;
+    virtual void configure_output(DisplayConfigurationOutputId id, bool used, geometry::Point top_left,
+                                  size_t mode_index, MirPixelFormat format, MirPowerMode power_mode) = 0;
 
 protected:
     DisplayConfiguration() = default;
 
=== modified file 'include/shared/mir_toolkit/client_types.h'
--- include/shared/mir_toolkit/client_types.h	2013-12-18 02:19:19 +0000
+++ include/shared/mir_toolkit/client_types.h	2014-01-10 10:21:28 +0000
@@ -230,7 +230,7 @@
 
     uint32_t num_output_formats;
     MirPixelFormat* output_formats;
-    uint32_t current_output_format;
+    MirPixelFormat current_format;
 
     uint32_t card_id;
     uint32_t output_id;
 
=== modified file 'include/test/mir_test_doubles/null_display_configuration.h'
--- include/test/mir_test_doubles/null_display_configuration.h	2013-09-12 21:36:55 +0000
+++ include/test/mir_test_doubles/null_display_configuration.h	2014-01-10 10:21:28 +0000
@@ -29,13 +29,13 @@
 {
 class NullDisplayConfiguration : public graphics::DisplayConfiguration
 {
-    void for_each_card(std::function<void(graphics::DisplayConfigurationCard const&)>) const
-    {
-    }
-    void for_each_output(std::function<void(graphics::DisplayConfigurationOutput const&)>) const
-    {
-    }
-    void configure_output(graphics::DisplayConfigurationOutputId, bool, geometry::Point, size_t, MirPowerMode) override
+    void for_each_card(std::function<void(graphics::DisplayConfigurationCard const&)>) const override
+    {
+    }
+    void for_each_output(std::function<void(graphics::DisplayConfigurationOutput const&)>) const override
+    {
+    }
+    void configure_output(graphics::DisplayConfigurationOutputId, bool, geometry::Point, size_t, MirPixelFormat, MirPowerMode) override
     {
     }
 };
 
=== modified file 'include/test/mir_test_doubles/stub_display_configuration.h'
--- include/test/mir_test_doubles/stub_display_configuration.h	2013-12-18 02:19:19 +0000
+++ include/test/mir_test_doubles/stub_display_configuration.h	2014-01-10 10:21:28 +0000
@@ -79,7 +79,7 @@
                 ((i % 2) == 0),
                 ((i % 2) == 1),
                 top_left,
-                mode_index, 1u,
+                mode_index, pfs[0],
                 mir_power_mode_off
             };
 
@@ -107,7 +107,8 @@
                 graphics::DisplayConfigurationOutputType::vga,
                 std::vector<MirPixelFormat>{mir_pixel_format_abgr_8888},
                 {{rect.size, 60.0}},
-                0, geometry::Size{}, true, true, rect.top_left, 0, 0, mir_power_mode_on
+                0, geometry::Size{}, true, true, rect.top_left, 0,
+                mir_pixel_format_abgr_8888, mir_power_mode_on
             };
 
             outputs.push_back(output);
@@ -122,13 +123,13 @@
         cards.push_back(card);
     }
 
-    void for_each_card(std::function<void(graphics::DisplayConfigurationCard const&)> f) const
+    void for_each_card(std::function<void(graphics::DisplayConfigurationCard const&)> f) const override
     {
         for (auto const& card : cards)
             f(card);
     }
 
-    void for_each_output(std::function<void(graphics::DisplayConfigurationOutput const&)> f) const
+    void for_each_output(std::function<void(graphics::DisplayConfigurationOutput const&)> f) const override
     {
         for (auto& disp : outputs)
         {
@@ -136,7 +137,7 @@
         }
     }
 
-    void configure_output(graphics::DisplayConfigurationOutputId, bool, geometry::Point, size_t, MirPowerMode)
+    void configure_output(graphics::DisplayConfigurationOutputId, bool, geometry::Point, size_t, MirPixelFormat, MirPowerMode) override
     {
     }
 
 
=== modified file 'src/client/CMakeLists.txt'
--- src/client/CMakeLists.txt	2013-12-20 18:08:18 +0000
+++ src/client/CMakeLists.txt	2014-01-10 10:21:28 +0000
@@ -69,7 +69,7 @@
   ${CLIENT_SOURCES}
 )
 
-set(MIRCLIENT_ABI 4)
+set(MIRCLIENT_ABI 5)
 
 set_target_properties(
   mirclient
 
=== modified file 'src/client/display_configuration.cpp'
--- src/client/display_configuration.cpp	2013-12-18 02:19:19 +0000
+++ src/client/display_configuration.cpp	2014-01-10 10:21:28 +0000
@@ -88,7 +88,7 @@
     {
         output.output_formats[i] = static_cast<MirPixelFormat>(msg.pixel_format(i));
     }
-    output.current_output_format = msg.current_format();
+    output.current_format = static_cast<MirPixelFormat>(msg.current_format());
 
     output.position_x = msg.position_x();
     output.position_y = msg.position_y();
 
=== modified file 'src/platform/graphics/android/android_display_configuration.cpp'
--- src/platform/graphics/android/android_display_configuration.cpp	2013-12-18 02:19:19 +0000
+++ src/platform/graphics/android/android_display_configuration.cpp	2014-01-10 10:21:28 +0000
@@ -24,14 +24,20 @@
         : configuration{mg::DisplayConfigurationOutputId{1},
                         mg::DisplayConfigurationCardId{0},
                         mg::DisplayConfigurationOutputType::lvds,
-                        {mir_pixel_format_abgr_8888},
+                        {
+                            mir_pixel_format_abgr_8888,
+                            mir_pixel_format_bgr_888,
+                            mir_pixel_format_xbgr_8888
+                        },
                         {mg::DisplayConfigurationMode{display_size,0.0f}},
                         0,
                         geom::Size{0,0},
                         true,
                         true,
                         geom::Point{0,0},
-                        0, 0, mir_power_mode_on},
+                        0,
+                        mir_pixel_format_abgr_8888,
+                        mir_power_mode_on},
           card{mg::DisplayConfigurationCardId{0}, 1}
 {
 }
@@ -62,7 +68,7 @@
     f(configuration);
 }
 
-void mga::AndroidDisplayConfiguration::configure_output(mg::DisplayConfigurationOutputId, bool, geom::Point, size_t, MirPowerMode power_mode)
+void mga::AndroidDisplayConfiguration::configure_output(mg::DisplayConfigurationOutputId, bool, geom::Point, size_t, MirPixelFormat, MirPowerMode power_mode)
 {
     configuration.power_mode = power_mode;
 }
 
=== modified file 'src/platform/graphics/android/android_display_configuration.h'
--- src/platform/graphics/android/android_display_configuration.h	2013-12-18 02:19:19 +0000
+++ src/platform/graphics/android/android_display_configuration.h	2014-01-10 10:21:28 +0000
@@ -35,9 +35,10 @@
 
     virtual ~AndroidDisplayConfiguration() = default;
 
-    void for_each_card(std::function<void(DisplayConfigurationCard const&)> f) const;
-    void for_each_output(std::function<void(DisplayConfigurationOutput const&)> f) const;
-    void configure_output(DisplayConfigurationOutputId, bool, geometry::Point, size_t, MirPowerMode power_mode);
+    void for_each_card(std::function<void(DisplayConfigurationCard const&)> f) const override;
+    void for_each_output(std::function<void(DisplayConfigurationOutput const&)> f) const override;
+    void configure_output(DisplayConfigurationOutputId id, bool used, geometry::Point top_left,
+                          size_t mode_index, MirPixelFormat format, MirPowerMode power_mode) override;
 
 private:
     DisplayConfigurationOutput configuration;
 
=== modified file 'src/platform/graphics/mesa/real_kms_display_configuration.cpp'
--- src/platform/graphics/mesa/real_kms_display_configuration.cpp	2013-12-18 02:19:19 +0000
+++ src/platform/graphics/mesa/real_kms_display_configuration.cpp	2014-01-10 10:21:28 +0000
@@ -18,6 +18,7 @@
 
 #include "real_kms_display_configuration.h"
 #include "drm_mode_resources.h"
+#include "mir/graphics/pixel_format_utils.h"
 
 #include <cmath>
 #include <limits>
@@ -65,6 +66,11 @@
     return static_cast<mg::DisplayConfigurationOutputType>(connector_type);
 }
 
+bool format_available_in_pixel_formats(MirPixelFormat format, mg::DisplayConfigurationOutput const& output)
+{
+    return output.pixel_formats.end() != find(output.pixel_formats.begin(), output.pixel_formats.end(), format);
+}
+
 }
 
 mgm::RealKMSDisplayConfiguration::RealKMSDisplayConfiguration(int drm_fd)
@@ -109,7 +115,7 @@
 void mgm::RealKMSDisplayConfiguration::configure_output(
     DisplayConfigurationOutputId id, bool used,
     geometry::Point top_left, size_t mode_index,
-    MirPowerMode power_mode)
+    MirPixelFormat format, MirPowerMode power_mode)
 {
     auto iter = find_output_with_id(id);
 
@@ -120,9 +126,16 @@
         if (used && mode_index >= output.modes.size())
             BOOST_THROW_EXCEPTION(std::runtime_error("Invalid mode_index for used output"));
 
+        if (used && !valid_pixel_format(format))
+            BOOST_THROW_EXCEPTION(std::runtime_error("Invalid format for used output"));
+
+        if (used && !format_available_in_pixel_formats(format, output))
+            BOOST_THROW_EXCEPTION(std::runtime_error("Format not available for used output"));
+
         output.used = used;
         output.top_left = top_left;
         output.current_mode_index = mode_index;
+        output.current_format = format;
         output.power_mode = power_mode;
     }
     else
@@ -164,7 +177,7 @@
     DRMModeResources resources{drm_fd};
 
     size_t max_outputs = std::min(resources.num_crtcs(), resources.num_connectors());
-    card = {mg::DisplayConfigurationCardId{0}, max_outputs};
+    card = {DisplayConfigurationCardId{0}, max_outputs};
 
     resources.for_each_connector([&](DRMModeConnectorUPtr connector)
     {
@@ -224,7 +237,8 @@
     {
         outputs.push_back({id, card_id, type, formats, modes, preferred_mode_index,
                            physical_size, connected, false, geom::Point(),
-                           current_mode_index, 0u, mir_power_mode_on});
+                           current_mode_index, mir_pixel_format_xrgb_8888,
+                           mir_power_mode_on});
     }
     else
     {
@@ -235,11 +249,12 @@
         output.physical_size_mm = physical_size;
         output.connected = connected;
         output.current_mode_index = current_mode_index;
+        output.current_format = mir_pixel_format_xrgb_8888;
     }
 }
 
 std::vector<mg::DisplayConfigurationOutput>::iterator
-mgm::RealKMSDisplayConfiguration::find_output_with_id(DisplayConfigurationOutputId id)
+mgm::RealKMSDisplayConfiguration::find_output_with_id(mg::DisplayConfigurationOutputId id)
 {
     return std::find_if(outputs.begin(), outputs.end(),
                         [id](DisplayConfigurationOutput const& output)
@@ -249,7 +264,7 @@
 }
 
 std::vector<mg::DisplayConfigurationOutput>::const_iterator
-mgm::RealKMSDisplayConfiguration::find_output_with_id(DisplayConfigurationOutputId id) const
+mgm::RealKMSDisplayConfiguration::find_output_with_id(mg::DisplayConfigurationOutputId id) const
 {
     return std::find_if(outputs.begin(), outputs.end(),
                         [id](DisplayConfigurationOutput const& output)
@@ -257,3 +272,4 @@
                             return output.id == id;
                         });
 }
+
 
=== modified file 'src/platform/graphics/mesa/real_kms_display_configuration.h'
--- src/platform/graphics/mesa/real_kms_display_configuration.h	2013-12-18 02:19:19 +0000
+++ src/platform/graphics/mesa/real_kms_display_configuration.h	2014-01-10 10:21:28 +0000
@@ -37,10 +37,10 @@
     RealKMSDisplayConfiguration(RealKMSDisplayConfiguration const& conf);
     RealKMSDisplayConfiguration& operator=(RealKMSDisplayConfiguration const& conf);
 
-    void for_each_card(std::function<void(DisplayConfigurationCard const&)> f) const;
-    void for_each_output(std::function<void(DisplayConfigurationOutput const&)> f) const;
-    void configure_output(DisplayConfigurationOutputId id, bool used,
-                          geometry::Point top_left, size_t mode_index, MirPowerMode power_mode);
+    void for_each_card(std::function<void(DisplayConfigurationCard const&)> f) const override;
+    void for_each_output(std::function<void(DisplayConfigurationOutput const&)> f) const override;
+    void configure_output(DisplayConfigurationOutputId id, bool used, geometry::Point top_left,
+                          size_t mode_index, MirPixelFormat format, MirPowerMode power_mode) override;
 
     uint32_t get_kms_connector_id(DisplayConfigurationOutputId id) const;
     size_t get_kms_mode_index(DisplayConfigurationOutputId id, size_t conf_mode_index) const;
 
=== modified file 'src/server/frontend/protobuf_buffer_packer.cpp'
--- src/server/frontend/protobuf_buffer_packer.cpp	2013-12-18 02:19:19 +0000
+++ src/server/frontend/protobuf_buffer_packer.cpp	2014-01-10 10:21:28 +0000
@@ -64,7 +64,7 @@
     protobuf_output.set_position_x(display_output.top_left.x.as_uint32_t());
     protobuf_output.set_position_y(display_output.top_left.y.as_uint32_t());
     protobuf_output.set_current_mode(display_output.current_mode_index);
-    protobuf_output.set_current_format(display_output.current_format_index);
+    protobuf_output.set_current_format(static_cast<uint32_t>(display_output.current_format));
     protobuf_output.set_power_mode(static_cast<uint32_t>(display_output.power_mode));
 }
 
 
=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2013-12-18 02:19:19 +0000
+++ src/server/frontend/session_mediator.cpp	2014-01-10 10:21:28 +0000
@@ -34,6 +34,7 @@
 #include "mir/graphics/platform.h"
 #include "mir/frontend/display_changer.h"
 #include "mir/graphics/display_configuration.h"
+#include "mir/graphics/pixel_format_utils.h"
 #include "mir/graphics/platform_ipc_package.h"
 #include "mir/frontend/client_constants.h"
 #include "mir/frontend/event_sink.h"
@@ -321,7 +322,9 @@
             mg::DisplayConfigurationOutputId output_id{static_cast<int>(output.output_id())};
             config->configure_output(output_id, output.used(),
                                      geom::Point{output.position_x(), output.position_y()},
-                                     output.current_mode(), static_cast<MirPowerMode>(output.power_mode()));
+                                     output.current_mode(),
+                                     static_cast<MirPixelFormat>(output.current_format()),
+                                     static_cast<MirPowerMode>(output.power_mode()));
         }
 
         display_changer->configure(session, config);
 
=== modified file 'src/server/graphics/default_display_configuration_policy.cpp'
--- src/server/graphics/default_display_configuration_policy.cpp	2013-12-18 02:19:19 +0000
+++ src/server/graphics/default_display_configuration_policy.cpp	2014-01-10 10:21:28 +0000
@@ -18,12 +18,52 @@
 
 #include "default_display_configuration_policy.h"
 #include "mir/graphics/display_configuration.h"
+#include "mir/graphics/pixel_format_utils.h"
 
 #include <unordered_map>
+#include <algorithm>
 
 namespace mg = mir::graphics;
 namespace geom = mir::geometry;
 
+namespace
+{
+size_t select_mode_index(size_t mode_index, std::vector<mg::DisplayConfigurationMode> const & modes)
+{
+    if (modes.empty())
+        return std::numeric_limits<size_t>::max();
+
+    if (mode_index >= modes.size())
+        return 0;
+
+    return mode_index;
+}
+
+MirPixelFormat select_opaque_format(MirPixelFormat format, std::vector<MirPixelFormat> const& formats)
+{
+    auto const format_in_formats = formats.end() != std::find(formats.begin(), formats.end(), format);
+
+    if (!mg::contains_alpha(format) && format_in_formats)
+        return format;
+
+    // format is either unavailable or transparent
+    auto const first_opaque = std::find_if_not(formats.begin(), formats.end(), mg::contains_alpha);
+
+    if (first_opaque != formats.end())
+        return *first_opaque;
+
+    // only tranparent options - allow choice if available
+    if (format_in_formats)
+        return format;
+
+    if (formats.size())
+        return formats.at(0);
+
+    return mir_pixel_format_invalid;
+}
+
+}
+
 void mg::DefaultDisplayConfigurationPolicy::apply_to(DisplayConfiguration& conf)
 {
     static MirPowerMode const default_power_state = mir_power_mode_on;
@@ -38,23 +78,22 @@
     conf.for_each_output(
         [&](DisplayConfigurationOutput const& conf_output)
         {
-            if (conf_output.connected && conf_output.modes.size() > 0 &&
-                available_outputs_for_card[conf_output.card_id] > 0)
-            {
-                size_t preferred_mode_index{conf_output.preferred_mode_index};
-                if (preferred_mode_index > conf_output.modes.size())
-                    preferred_mode_index = 0;
-
-                conf.configure_output(conf_output.id, true, geom::Point(),
-                                      preferred_mode_index, default_power_state);
-
-                --available_outputs_for_card[conf_output.card_id];
-            }
-            else
+            if (!conf_output.connected || conf_output.modes.empty() ||
+                available_outputs_for_card[conf_output.card_id] == 0)
             {
                 conf.configure_output(conf_output.id, false, conf_output.top_left,
-                                      conf_output.current_mode_index, default_power_state);
+                                      conf_output.current_mode_index, conf_output.current_format,
+                                      default_power_state);
+                return;
             }
+
+            size_t preferred_mode_index{select_mode_index(conf_output.preferred_mode_index, conf_output.modes)};
+            MirPixelFormat format{select_opaque_format(conf_output.current_format, conf_output.pixel_formats)};
+
+            conf.configure_output(conf_output.id, true, geom::Point(), preferred_mode_index,
+                                  format, default_power_state);
+
+            --available_outputs_for_card[conf_output.card_id];
         });
 }
 
 
=== modified file 'src/server/graphics/nested/nested_display.cpp'
--- src/server/graphics/nested/nested_display.cpp	2013-12-20 05:06:28 +0000
+++ src/server/graphics/nested/nested_display.cpp	2014-01-10 10:21:28 +0000
@@ -22,8 +22,10 @@
 #include "mir_api_wrappers.h"
 
 #include "mir/geometry/rectangle.h"
+#include "mir/graphics/pixel_format_utils.h"
 #include "mir/graphics/gl_context.h"
 #include "mir/graphics/surfaceless_egl_context.h"
+#include "mir/graphics/display_configuration_policy.h"
 #include "host_connection.h"
 
 #include <boost/throw_exception.hpp>
@@ -34,45 +36,8 @@
 namespace mgnw = mir::graphics::nested::mir_api_wrappers;
 namespace geom = mir::geometry;
 
-namespace
-{
-
-MirPixelFormat find_opaque_surface_format(MirConnection* connection)
-{
-    static unsigned const max_formats = 32;
-    MirPixelFormat formats[max_formats];
-    unsigned int valid_formats;
-
-    mir_connection_get_available_surface_formats(connection, formats,
-                                                 max_formats, &valid_formats);
-
-    // Find an opaque surface format
-    for (auto f = formats; f != formats+valid_formats; ++f)
-    {
-        if (*f == mir_pixel_format_xbgr_8888 ||
-            *f == mir_pixel_format_xrgb_8888)
-        {
-            return *f;
-        }
-    }
-
-    BOOST_THROW_EXCEPTION(
-        std::runtime_error("Nested Mir failed to find an opaque surface format"));
-}
-
-}
-
-EGLint const mgn::detail::nested_egl_config_attribs[] = {
-    EGL_SURFACE_TYPE, EGL_WINDOW_BIT,
-    EGL_RED_SIZE, 8,
-    EGL_GREEN_SIZE, 8,
-    EGL_BLUE_SIZE, 8,
-    EGL_ALPHA_SIZE, 0,
-    EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
-    EGL_NONE
-};
-
-EGLint const mgn::detail::nested_egl_context_attribs[] = {
+EGLint const mgn::detail::nested_egl_context_attribs[] =
+{
     EGL_CONTEXT_CLIENT_VERSION, 2,
     EGL_NONE
 };
@@ -93,6 +58,8 @@
 }
 
 mgn::detail::EGLDisplayHandle::EGLDisplayHandle(MirConnection* connection)
+    : egl_display(EGL_NO_DISPLAY),
+    egl_context_(EGL_NO_CONTEXT)
 {
     auto const native_display = (EGLNativeDisplayType) mir_connection_get_egl_native_display(connection);
     egl_display = eglGetDisplay(native_display);
@@ -100,7 +67,7 @@
         BOOST_THROW_EXCEPTION(std::runtime_error("Nested Mir Display Error: Failed to fetch EGL display."));
 }
 
-void mgn::detail::EGLDisplayHandle::initialize()
+void mgn::detail::EGLDisplayHandle::initialize(MirPixelFormat format)
 {
     int major;
     int minor;
@@ -110,17 +77,28 @@
         BOOST_THROW_EXCEPTION(std::runtime_error("Nested Mir Display Error: Failed to initialize EGL."));
     }
 
-    egl_context_ = eglCreateContext(egl_display, choose_config(detail::nested_egl_config_attribs), EGL_NO_CONTEXT, detail::nested_egl_context_attribs);
+    egl_context_ = eglCreateContext(egl_display, choose_windowed_es_config(format), EGL_NO_CONTEXT, detail::nested_egl_context_attribs);
+
     if (egl_context_ == EGL_NO_CONTEXT)
         BOOST_THROW_EXCEPTION(std::runtime_error("Failed to create shared EGL context"));
 }
 
-EGLConfig mgn::detail::EGLDisplayHandle::choose_config(const EGLint attrib_list[]) const
+EGLConfig mgn::detail::EGLDisplayHandle::choose_windowed_es_config(MirPixelFormat format) const
 {
+    EGLint const nested_egl_config_attribs[] =
+    {
+        EGL_SURFACE_TYPE, EGL_WINDOW_BIT,
+        EGL_RED_SIZE, mg::red_channel_depth(format),
+        EGL_GREEN_SIZE, mg::green_channel_depth(format),
+        EGL_BLUE_SIZE, mg::blue_channel_depth(format),
+        EGL_ALPHA_SIZE, mg::alpha_channel_depth(format),
+        EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
+        EGL_NONE
+    };
     EGLConfig result;
     int n;
 
-    int res = eglChooseConfig(egl_display, attrib_list, &result, 1, &n);
+    int res = eglChooseConfig(egl_display, nested_egl_config_attribs, &result, 1, &n);
     if ((res != EGL_TRUE) || (n != 1))
         BOOST_THROW_EXCEPTION(std::runtime_error("Nested Mir Display Error: Failed to choose EGL configuration."));
 
@@ -149,17 +127,18 @@
 mgn::NestedDisplay::NestedDisplay(
     std::shared_ptr<HostConnection> const& connection,
     std::shared_ptr<input::EventFilter> const& event_handler,
-    std::shared_ptr<mg::DisplayReport> const& display_report) :
+    std::shared_ptr<mg::DisplayReport> const& display_report,
+    std::shared_ptr<mg::DisplayConfigurationPolicy> const& initial_conf_policy) :
     connection{connection},
     event_handler{event_handler},
     display_report{display_report},
     egl_display{*connection},
-    egl_pixel_format{find_opaque_surface_format(*connection)},
     outputs{}
 {
-    egl_display.initialize();
-    eglMakeCurrent(egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE, egl_display.egl_context());
-    configure(*configuration());
+
+    std::shared_ptr<DisplayConfiguration> conf(configuration());
+    initial_conf_policy->apply_to(*conf);
+    configure(*conf);
 }
 
 mgn::NestedDisplay::~NestedDisplay() noexcept
@@ -178,6 +157,14 @@
     return std::make_shared<NestedDisplayConfiguration>(mir_connection_create_display_config(*connection));
 }
 
+void mgn::NestedDisplay::complete_display_initialization(MirPixelFormat format)
+{
+    if (egl_display.egl_context() != EGL_NO_CONTEXT)  return;
+
+    egl_display.initialize(format);
+    eglMakeCurrent(egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE, egl_display.egl_context());
+}
+
 void mgn::NestedDisplay::configure(mg::DisplayConfiguration const& configuration)
 {
     decltype(outputs) result;
@@ -193,13 +180,16 @@
                 geometry::Rectangle const area{output.top_left, output.modes[output.current_mode_index].size};
 
                 auto const& egl_display_mode = output.modes[output.current_mode_index];
+                auto const& egl_config_format = output.current_format;
+
+                complete_display_initialization(egl_config_format);
 
                 MirSurfaceParameters const request_params =
                     {
                         "Mir nested display",
                         egl_display_mode.size.width.as_int(),
                         egl_display_mode.size.height.as_int(),
-                        egl_pixel_format,
+                        egl_config_format,
                         mir_buffer_usage_hardware,
                         static_cast<uint32_t>(output.id.as_value())
                     };
@@ -213,7 +203,8 @@
                     egl_display,
                     mir_surface,
                     area,
-                    event_handler);
+                    event_handler,
+                    output.current_format);
             }
         });
 
@@ -274,13 +265,7 @@
     return std::weak_ptr<Cursor>();
 }
 
-namespace
-{
-}
-
 std::unique_ptr<mg::GLContext> mgn::NestedDisplay::create_gl_context()
 {
-    return std::unique_ptr<mg::GLContext>{new SurfacelessEGLContext(egl_display,
-                                                                    detail::nested_egl_config_attribs,
-                                                                    EGL_NO_CONTEXT)};
+    return std::unique_ptr<mg::GLContext>{new SurfacelessEGLContext(egl_display, EGL_NO_CONTEXT)};
 }
 
=== modified file 'src/server/graphics/nested/nested_display.h'
--- src/server/graphics/nested/nested_display.h	2013-12-18 02:19:19 +0000
+++ src/server/graphics/nested/nested_display.h	2014-01-10 10:21:28 +0000
@@ -42,6 +42,7 @@
 {
 class DisplayReport;
 class DisplayBuffer;
+class DisplayConfigurationPolicy;
 
 namespace nested
 {
@@ -67,8 +68,8 @@
     explicit EGLDisplayHandle(MirConnection* connection);
     ~EGLDisplayHandle() noexcept;
 
-    void initialize();
-    EGLConfig choose_config(const EGLint attrib_list[]) const;
+    void initialize(MirPixelFormat format);
+    EGLConfig choose_windowed_es_config(MirPixelFormat format) const;
     EGLNativeWindowType native_window(EGLConfig egl_config, MirSurface* mir_surface) const;
     EGLContext egl_context() const;
     operator EGLDisplay() const { return egl_display; }
@@ -83,7 +84,6 @@
 
 class NestedOutput;
 
-extern EGLint const nested_egl_config_attribs[];
 extern EGLint const nested_egl_context_attribs[];
 }
 
@@ -95,7 +95,8 @@
     NestedDisplay(
         std::shared_ptr<HostConnection> const& connection,
         std::shared_ptr<input::EventFilter> const& event_handler,
-        std::shared_ptr<DisplayReport> const& display_report);
+        std::shared_ptr<DisplayReport> const& display_report,
+        std::shared_ptr<DisplayConfigurationPolicy> const& conf_policy);
 
     ~NestedDisplay() noexcept;
 
@@ -124,11 +125,11 @@
     std::shared_ptr<input::EventFilter> const event_handler;
     std::shared_ptr<DisplayReport> const display_report;
     detail::EGLDisplayHandle egl_display;
-    MirPixelFormat const egl_pixel_format;
 
     std::mutex outputs_mutex;
     std::unordered_map<DisplayConfigurationOutputId, std::shared_ptr<detail::NestedOutput>> outputs;
     DisplayConfigurationChangeHandler my_conf_change_handler;
+    void complete_display_initialization(MirPixelFormat format);
 };
 
 }
 
=== modified file 'src/server/graphics/nested/nested_display_configuration.cpp'
--- src/server/graphics/nested/nested_display_configuration.cpp	2013-12-18 02:19:19 +0000
+++ src/server/graphics/nested/nested_display_configuration.cpp	2014-01-10 10:21:28 +0000
@@ -18,15 +18,26 @@
 
 #include "nested_display_configuration.h"
 
-namespace mg = mir::graphics;
-namespace mgn = mg::nested;
+#include "mir/graphics/pixel_format_utils.h"
 
 #include <boost/throw_exception.hpp>
 
 #include <stdexcept>
 #include <algorithm>
 
-namespace mgn = mir::graphics::nested;
+
+namespace mg = mir::graphics;
+namespace mgn = mg::nested;
+
+namespace
+{
+bool format_valid_for_output(MirDisplayOutput const& output, MirPixelFormat format)
+{
+    MirPixelFormat * end = output.output_formats + output.num_output_formats;
+    return end != std::find(output.output_formats, end, format);
+}
+
+}
 
 mgn::NestedDisplayConfiguration::NestedDisplayConfiguration(MirDisplayConfiguration* connection) :
 display_config{connection}
@@ -77,7 +88,7 @@
                 !!mir_output.used,
                 geometry::Point{mir_output.position_x, mir_output.position_y},
                 mir_output.current_mode,
-                mir_output.current_output_format,
+                mir_output.current_format,
                 mir_output.power_mode
             };
 
@@ -86,7 +97,7 @@
 }
 
 void mgn::NestedDisplayConfiguration::configure_output(DisplayConfigurationOutputId id, bool used,
-    geometry::Point top_left, size_t mode_index, MirPowerMode power_mode)
+    geometry::Point top_left, size_t mode_index, MirPixelFormat format, MirPowerMode power_mode)
 {
     for (auto mir_output = display_config->outputs;
         mir_output != display_config->outputs+display_config->num_outputs;
@@ -97,13 +108,21 @@
             if (used && mode_index >= mir_output->num_modes)
                 BOOST_THROW_EXCEPTION(std::runtime_error("Invalid mode_index for used output"));
 
+            if (used && !mg::valid_pixel_format(format))
+                BOOST_THROW_EXCEPTION(std::runtime_error("Invalid format for used output"));
+
+            if (used && !format_valid_for_output(*mir_output, format))
+                BOOST_THROW_EXCEPTION(std::runtime_error("Format not available for used output"));
+
             mir_output->used = used;
             mir_output->position_x = top_left.x.as_uint32_t();
             mir_output->position_y = top_left.y.as_uint32_t();
             mir_output->current_mode = mode_index;
+            mir_output->current_format = format;
             mir_output->power_mode = static_cast<MirPowerMode>(power_mode);
             return;
         }
     }
     BOOST_THROW_EXCEPTION(std::runtime_error("Trying to configure invalid output"));
 }
+
 
=== modified file 'src/server/graphics/nested/nested_display_configuration.h'
--- src/server/graphics/nested/nested_display_configuration.h	2013-09-12 21:36:55 +0000
+++ src/server/graphics/nested/nested_display_configuration.h	2014-01-10 10:21:28 +0000
@@ -34,11 +34,11 @@
     explicit NestedDisplayConfiguration(MirDisplayConfiguration* display_config);
     virtual ~NestedDisplayConfiguration() noexcept;
 
-    void for_each_card(std::function<void(DisplayConfigurationCard const&)>) const;
-    void for_each_output(std::function<void(DisplayConfigurationOutput const&)>) const;
+    void for_each_card(std::function<void(DisplayConfigurationCard const&)>) const override;
+    void for_each_output(std::function<void(DisplayConfigurationOutput const&)>) const override;
 
     void configure_output(DisplayConfigurationOutputId id, bool used, geometry::Point top_left, size_t mode_index,
-                          MirPowerMode power_mode);
+                          MirPixelFormat format, MirPowerMode power_mode) override;
 
     operator MirDisplayConfiguration*() const { return display_config; }
 private:
 
=== modified file 'src/server/graphics/nested/nested_output.cpp'
--- src/server/graphics/nested/nested_output.cpp	2013-09-23 13:37:44 +0000
+++ src/server/graphics/nested/nested_output.cpp	2014-01-10 10:21:28 +0000
@@ -41,10 +41,11 @@
     EGLDisplayHandle const& egl_display,
     MirSurface* mir_surface,
     geometry::Rectangle const& area,
-    std::shared_ptr<input::EventFilter> const& event_handler) :
+    std::shared_ptr<input::EventFilter> const& event_handler,
+    MirPixelFormat preferred_format) :
     egl_display(egl_display),
     mir_surface{mir_surface},
-    egl_config{egl_display.choose_config(nested_egl_config_attribs)},
+    egl_config{egl_display.choose_windowed_es_config(preferred_format)},
     egl_context{egl_display, eglCreateContext(egl_display, egl_config, egl_display.egl_context(), nested_egl_context_attribs)},
     area{area.top_left, area.size},
     event_handler{event_handler},
 
=== modified file 'src/server/graphics/nested/nested_output.h'
--- src/server/graphics/nested/nested_output.h	2013-09-23 13:37:44 +0000
+++ src/server/graphics/nested/nested_output.h	2014-01-10 10:21:28 +0000
@@ -54,7 +54,8 @@
         EGLDisplayHandle const& egl_display,
         MirSurface* mir_surface,
         geometry::Rectangle const& area,
-        std::shared_ptr<input::EventFilter> const& event_handler);
+        std::shared_ptr<input::EventFilter> const& event_handler,
+        MirPixelFormat preferred_format);
 
     ~NestedOutput() noexcept;
 
 
=== modified file 'src/server/graphics/nested/nested_platform.cpp'
--- src/server/graphics/nested/nested_platform.cpp	2013-12-18 02:19:19 +0000
+++ src/server/graphics/nested/nested_platform.cpp	2014-01-10 10:21:28 +0000
@@ -113,9 +113,9 @@
     return native_platform->create_buffer_allocator(buffer_initializer);
 }
 
-std::shared_ptr<mg::Display> mgn::NestedPlatform::create_display(std::shared_ptr<mg::DisplayConfigurationPolicy> const& /*initial_conf_policy*/)
+std::shared_ptr<mg::Display> mgn::NestedPlatform::create_display(std::shared_ptr<mg::DisplayConfigurationPolicy> const& conf_policy)
 {
-    return std::make_shared<mgn::NestedDisplay>(connection, event_handler, display_report);
+    return std::make_shared<mgn::NestedDisplay>(connection, event_handler, display_report, conf_policy);
 }
 
 std::shared_ptr<mg::PlatformIPCPackage> mgn::NestedPlatform::get_ipc_package()
 
=== modified file 'src/server/graphics/nested/nested_platform.h'
--- src/server/graphics/nested/nested_platform.h	2013-12-18 02:19:19 +0000
+++ src/server/graphics/nested/nested_platform.h	2014-01-10 10:21:28 +0000
@@ -42,12 +42,12 @@
 
     ~NestedPlatform() noexcept;
     std::shared_ptr<GraphicBufferAllocator> create_buffer_allocator(
-            std::shared_ptr<BufferInitializer> const& buffer_initializer);
+            std::shared_ptr<BufferInitializer> const& buffer_initializer) override;
     std::shared_ptr<Display> create_display(
-            std::shared_ptr<DisplayConfigurationPolicy> const& initial_conf_policy);
-    std::shared_ptr<PlatformIPCPackage> get_ipc_package();
-    std::shared_ptr<InternalClient> create_internal_client();
-    void fill_ipc_package(BufferIPCPacker* packer, Buffer const* Buffer) const;
+            std::shared_ptr<DisplayConfigurationPolicy> const& initial_conf_policy) override;
+    std::shared_ptr<PlatformIPCPackage> get_ipc_package() override;
+    std::shared_ptr<InternalClient> create_internal_client() override;
+    void fill_ipc_package(BufferIPCPacker* packer, Buffer const* Buffer) const override;
     EGLNativeDisplayType egl_native_display() const;
 
 private:
 
=== modified file 'src/server/graphics/offscreen/display_configuration.cpp'
--- src/server/graphics/offscreen/display_configuration.cpp	2013-12-18 02:19:19 +0000
+++ src/server/graphics/offscreen/display_configuration.cpp	2014-01-10 10:21:28 +0000
@@ -31,7 +31,9 @@
                  true,
                  true,
                  geom::Point{0,0},
-                 0, 0, mir_power_mode_on},
+                 0,
+                 mir_pixel_format_xrgb_8888,
+                 mir_power_mode_on},
           card{mg::DisplayConfigurationCardId{0}, 1}
 {
 }
@@ -67,6 +69,6 @@
 }
 
 void mgo::DisplayConfiguration::configure_output(
-    mg::DisplayConfigurationOutputId, bool, geom::Point, size_t, MirPowerMode)
+    mg::DisplayConfigurationOutputId, bool, geom::Point, size_t, MirPixelFormat, MirPowerMode)
 {
 }
 
=== modified file 'src/server/graphics/offscreen/display_configuration.h'
--- src/server/graphics/offscreen/display_configuration.h	2013-11-12 17:23:53 +0000
+++ src/server/graphics/offscreen/display_configuration.h	2014-01-10 10:21:28 +0000
@@ -33,10 +33,10 @@
     DisplayConfiguration(DisplayConfiguration const& other);
     DisplayConfiguration& operator=(DisplayConfiguration const& other);
 
-    void for_each_card(std::function<void(DisplayConfigurationCard const&)> f) const;
-    void for_each_output(std::function<void(DisplayConfigurationOutput const&)> f) const;
-    void configure_output(DisplayConfigurationOutputId, bool, geometry::Point,
-                          size_t, MirPowerMode power_mode);
+    void for_each_card(std::function<void(DisplayConfigurationCard const&)> f) const override;
+    void for_each_output(std::function<void(DisplayConfigurationOutput const&)> f) const override;
+    virtual void configure_output(DisplayConfigurationOutputId id, bool used, geometry::Point top_left,
+                                  size_t mode_index, MirPixelFormat format, MirPowerMode power_mode) override;
 
 private:
     DisplayConfigurationOutput output;
 
=== modified file 'src/server/scene/mediating_display_changer.cpp'
--- src/server/scene/mediating_display_changer.cpp	2013-12-18 02:19:19 +0000
+++ src/server/scene/mediating_display_changer.cpp	2014-01-10 10:21:28 +0000
@@ -136,6 +136,7 @@
             conf->configure_output(output.id, output.used,
                                    output.top_left,
                                    output.current_mode_index,
+                                   output.current_format,
                                    mir_power_mode_on);
         }
     });
 
=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2013-12-18 02:19:19 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2014-01-10 10:21:28 +0000
@@ -885,7 +885,7 @@
                 MirDisplayOutput* disp = &configuration->outputs[i];
                 ASSERT_NE(nullptr, disp);
                 EXPECT_GE(disp->num_modes, disp->current_mode);
-                EXPECT_GE(disp->num_output_formats, disp->current_output_format);
+                EXPECT_GE(disp->num_output_formats, disp->current_format);
             }
 
             mir_display_config_destroy(configuration);
 
=== modified file 'tests/mir_test/display_config_matchers.cpp'
--- tests/mir_test/display_config_matchers.cpp	2013-12-18 02:19:19 +0000
+++ tests/mir_test/display_config_matchers.cpp	2014-01-10 10:21:28 +0000
@@ -67,7 +67,7 @@
                 geom::Point{protobuf_output.position_x(),
                             protobuf_output.position_y()},
                 protobuf_output.current_mode(),
-                protobuf_output.current_format(),
+                static_cast<MirPixelFormat>(protobuf_output.current_format()),
                 static_cast<MirPowerMode>(protobuf_output.power_mode())
             };
 
@@ -132,7 +132,7 @@
                 geom::Point{client_output.position_x,
                             client_output.position_y},
                 client_output.current_mode,
-                client_output.current_output_format,
+                client_output.current_format,
                 static_cast<MirPowerMode>(client_output.power_mode)
             };
 
@@ -162,20 +162,20 @@
         }
     }
 
-    void for_each_card(std::function<void(mg::DisplayConfigurationCard const&)> f) const
+    void for_each_card(std::function<void(mg::DisplayConfigurationCard const&)> f) const override
     {
         for (auto const& card : cards)
             f(card);
     }
 
-    void for_each_output(std::function<void(mg::DisplayConfigurationOutput const&)> f) const
+    void for_each_output(std::function<void(mg::DisplayConfigurationOutput const&)> f) const override
     {
         for (auto const& output : outputs)
             f(output);
     }
 
     void configure_output(mg::DisplayConfigurationOutputId, bool,
-                          geom::Point, size_t, MirPowerMode)
+                          geom::Point, size_t, MirPixelFormat, MirPowerMode) override
     {
     }
 
 
=== modified file 'tests/mir_test_framework/stubbed_server_configuration.cpp'
--- tests/mir_test_framework/stubbed_server_configuration.cpp	2014-01-07 03:30:47 +0000
+++ tests/mir_test_framework/stubbed_server_configuration.cpp	2014-01-10 10:21:28 +0000
@@ -132,7 +132,7 @@
             mg::DisplayConfigurationCardId{0},
             mg::DisplayConfigurationOutputType::vga,
             std::vector<MirPixelFormat>{mir_pixel_format_abgr_8888},
-            modes, 0, geom::Size{}, true, true, geom::Point{0,0}, 0, 0, mir_power_mode_on};
+            modes, 0, geom::Size{}, true, true, geom::Point{0,0}, 0, mir_pixel_format_abgr_8888, mir_power_mode_on};
 
         f(dummy_output_config);
     }
 
=== modified file 'tests/unit-tests/frontend/test_session_mediator.cpp'
--- tests/unit-tests/frontend/test_session_mediator.cpp	2013-12-18 02:19:19 +0000
+++ tests/unit-tests/frontend/test_session_mediator.cpp	2014-01-10 10:21:28 +0000
@@ -83,7 +83,7 @@
 {
     MOCK_CONST_METHOD1(for_each_card, void(std::function<void(mg::DisplayConfigurationCard const&)>));
     MOCK_CONST_METHOD1(for_each_output, void(std::function<void(mg::DisplayConfigurationOutput const&)>));
-    MOCK_METHOD5(configure_output, void(mg::DisplayConfigurationOutputId, bool, geom::Point, size_t, MirPowerMode));
+    MOCK_METHOD6(configure_output, void(mg::DisplayConfigurationOutputId, bool, geom::Point, size_t, MirPixelFormat, MirPowerMode));
 };
 
 }
@@ -555,6 +555,8 @@
     bool used0 = false, used1 = true;
     geom::Point pt0{44,22}, pt1{3,2};
     size_t mode_index0 = 1, mode_index1 = 3;
+    MirPixelFormat format0{mir_pixel_format_invalid};
+    MirPixelFormat format1{mir_pixel_format_argb_8888};
     mg::DisplayConfigurationOutputId id0{6}, id1{3};
 
     NiceMock<MockConfig> mock_display_config;
@@ -568,9 +570,9 @@
     EXPECT_CALL(*mock_display_selector, active_configuration())
         .InSequence(seq)
         .WillOnce(Return(mt::fake_shared(mock_display_config)));
-    EXPECT_CALL(mock_display_config, configure_output(id0, used0, pt0, mode_index0,  mir_power_mode_on))
+    EXPECT_CALL(mock_display_config, configure_output(id0, used0, pt0, mode_index0, format0, mir_power_mode_on))
         .InSequence(seq);
-    EXPECT_CALL(mock_display_config, configure_output(id1, used1, pt1, mode_index1, mir_power_mode_off))
+    EXPECT_CALL(mock_display_config, configure_output(id1, used1, pt1, mode_index1, format1, mir_power_mode_off))
         .InSequence(seq);
     EXPECT_CALL(*mock_display_selector, configure(_,_))
         .InSequence(seq);
@@ -593,6 +595,7 @@
     disp0->set_position_x(pt0.x.as_uint32_t());
     disp0->set_position_y(pt0.y.as_uint32_t());
     disp0->set_current_mode(mode_index0);
+    disp0->set_current_format(format0);
     disp0->set_power_mode(static_cast<uint32_t>(mir_power_mode_on));
 
     auto disp1 = configuration.add_display_output();
@@ -601,6 +604,7 @@
     disp1->set_position_x(pt1.x.as_uint32_t());
     disp1->set_position_y(pt1.y.as_uint32_t());
     disp1->set_current_mode(mode_index1);
+    disp1->set_current_format(format1);
     disp1->set_power_mode(static_cast<uint32_t>(mir_power_mode_off));
 
     session_mediator.configure_display(nullptr, &configuration,
 
=== modified file 'tests/unit-tests/graphics/android/test_android_fb.cpp'
--- tests/unit-tests/graphics/android/test_android_fb.cpp	2013-12-18 02:19:19 +0000
+++ tests/unit-tests/graphics/android/test_android_fb.cpp	2014-01-10 10:21:28 +0000
@@ -256,28 +256,28 @@
     configuration->for_each_output([&](mg::DisplayConfigurationOutput const& output)
     {
         configuration->configure_output(
-            output.id, output.used, output.top_left, output.current_mode_index, mir_power_mode_on);
-    });
-    display.configure(*configuration);
-
-    configuration->for_each_output([&](mg::DisplayConfigurationOutput const& output)
-    {
-        configuration->configure_output(
-            output.id, output.used, output.top_left, output.current_mode_index, mir_power_mode_standby);
-    });
-    display.configure(*configuration);
-
-    configuration->for_each_output([&](mg::DisplayConfigurationOutput const& output)
-    {
-        configuration->configure_output(
-            output.id, output.used, output.top_left, output.current_mode_index, mir_power_mode_off);
-    });
-    display.configure(*configuration);
-
-    configuration->for_each_output([&](mg::DisplayConfigurationOutput const& output)
-    {
-        configuration->configure_output(
-            output.id, output.used, output.top_left, output.current_mode_index, mir_power_mode_suspend);
+            output.id, output.used, output.top_left, output.current_mode_index, output.current_format, mir_power_mode_on);
+    });
+    display.configure(*configuration);
+
+    configuration->for_each_output([&](mg::DisplayConfigurationOutput const& output)
+    {
+        configuration->configure_output(
+            output.id, output.used, output.top_left, output.current_mode_index, output.current_format, mir_power_mode_standby);
+    });
+    display.configure(*configuration);
+
+    configuration->for_each_output([&](mg::DisplayConfigurationOutput const& output)
+    {
+        configuration->configure_output(
+            output.id, output.used, output.top_left, output.current_mode_index, output.current_format, mir_power_mode_off);
+    });
+    display.configure(*configuration);
+
+    configuration->for_each_output([&](mg::DisplayConfigurationOutput const& output)
+    {
+        configuration->configure_output(
+            output.id, output.used, output.top_left, output.current_mode_index, output.current_format, mir_power_mode_suspend);
     });
     display.configure(*configuration);
 }
 
=== modified file 'tests/unit-tests/graphics/mesa/test_cursor.cpp'
--- tests/unit-tests/graphics/mesa/test_cursor.cpp	2013-12-18 02:19:19 +0000
+++ tests/unit-tests/graphics/mesa/test_cursor.cpp	2014-01-10 10:21:28 +0000
@@ -105,7 +105,7 @@
                 true,
                 geom::Point{0, 0},
                 1,
-                0,
+                mir_pixel_format_invalid,
                 mir_power_mode_on
             });
         outputs.push_back(
@@ -124,33 +124,33 @@
                 true,
                 geom::Point{100, 50},
                 0,
-                0,
+                mir_pixel_format_invalid,
                 mir_power_mode_on
             });
     }
 
-    void for_each_card(std::function<void(mg::DisplayConfigurationCard const&)> f) const
+    void for_each_card(std::function<void(mg::DisplayConfigurationCard const&)> f) const override
     {
         f({card_id, outputs.size()});
     }
 
-    void for_each_output(std::function<void(mg::DisplayConfigurationOutput const&)> f) const
+    void for_each_output(std::function<void(mg::DisplayConfigurationOutput const&)> f) const override
     {
         for (auto const& output : outputs)
             f(output);
     }
 
     void configure_output(mg::DisplayConfigurationOutputId, bool,
-                          geom::Point, size_t, MirPowerMode)
+                          geom::Point, size_t, MirPixelFormat, MirPowerMode) override
     {
     }
 
-    uint32_t get_kms_connector_id(mg::DisplayConfigurationOutputId id) const
+    uint32_t get_kms_connector_id(mg::DisplayConfigurationOutputId id) const override
     {
         return id.as_value();
     }
 
-    size_t get_kms_mode_index(mg::DisplayConfigurationOutputId, size_t conf_mode_index) const
+    size_t get_kms_mode_index(mg::DisplayConfigurationOutputId, size_t conf_mode_index) const override
     {
         return conf_mode_index;
     }
 
=== modified file 'tests/unit-tests/graphics/mesa/test_display_configuration.cpp'
--- tests/unit-tests/graphics/mesa/test_display_configuration.cpp	2013-12-18 02:19:19 +0000
+++ tests/unit-tests/graphics/mesa/test_display_configuration.cpp	2014-01-10 10:21:28 +0000
@@ -197,7 +197,7 @@
             true,
             geom::Point(),
             1,
-            0,
+            mir_pixel_format_invalid,
             mir_power_mode_on
         },
         {
@@ -212,7 +212,7 @@
             false,
             geom::Point(),
             std::numeric_limits<size_t>::max(),
-            std::numeric_limits<size_t>::max(),
+            mir_pixel_format_invalid,
             mir_power_mode_on
         },
         {
@@ -227,7 +227,7 @@
             false,
             geom::Point(),
             std::numeric_limits<size_t>::max(),
-            std::numeric_limits<size_t>::max(),
+            mir_pixel_format_invalid,
             mir_power_mode_on
         }
     };
@@ -381,7 +381,7 @@
             true,
             geom::Point(),
             1,
-            0,
+            mir_pixel_format_invalid,
             mir_power_mode_on
         },
         {
@@ -396,7 +396,7 @@
             false,
             geom::Point(),
             std::numeric_limits<size_t>::max(),
-            std::numeric_limits<size_t>::max(),
+            mir_pixel_format_invalid,
             mir_power_mode_on
         },
     };
@@ -415,7 +415,7 @@
             true,
             geom::Point(),
             std::numeric_limits<size_t>::max(),
-            std::numeric_limits<size_t>::max(),
+            mir_pixel_format_invalid,
             mir_power_mode_on
         },
         {
@@ -430,7 +430,7 @@
             false,
             geom::Point(),
             1,
-            0,
+            mir_pixel_format_invalid,
             mir_power_mode_on
         },
     };
 
=== modified file 'tests/unit-tests/graphics/mesa/test_display_multi_monitor.cpp'
--- tests/unit-tests/graphics/mesa/test_display_multi_monitor.cpp	2013-12-18 02:19:19 +0000
+++ tests/unit-tests/graphics/mesa/test_display_multi_monitor.cpp	2014-01-10 10:21:28 +0000
@@ -57,12 +57,16 @@
                 if (conf_output.connected && conf_output.modes.size() > 0)
                 {
                     conf.configure_output(conf_output.id, true, geom::Point{0, 0},
-                                          conf_output.preferred_mode_index, mir_power_mode_on);
+                                          conf_output.preferred_mode_index,
+                                          conf_output.current_format,
+                                          mir_power_mode_on);
                 }
                 else
                 {
                     conf.configure_output(conf_output.id, false, conf_output.top_left,
-                                          conf_output.current_mode_index, mir_power_mode_on);
+                                          conf_output.current_mode_index,
+                                          conf_output.current_format,
+                                          mir_power_mode_on);
                 }
             });
     }
@@ -81,13 +85,17 @@
                 if (conf_output.connected && conf_output.modes.size() > 0)
                 {
                     conf.configure_output(conf_output.id, true, geom::Point{max_x, 0},
-                                          conf_output.preferred_mode_index, mir_power_mode_on);
+                                          conf_output.preferred_mode_index,
+                                          conf_output.current_format,
+                                          mir_power_mode_on);
                     max_x += conf_output.modes[conf_output.preferred_mode_index].size.width.as_int();
                 }
                 else
                 {
                     conf.configure_output(conf_output.id, false, conf_output.top_left,
-                                          conf_output.current_mode_index, mir_power_mode_on);
+                                          conf_output.current_mode_index,
+                                          conf_output.current_format,
+                                          mir_power_mode_on);
                 }
             });
     }
@@ -472,7 +480,8 @@
         [&](mg::DisplayConfigurationOutput const& conf_output)
         {
             conf->configure_output(conf_output.id, false, conf_output.top_left,
-                                   conf_output.preferred_mode_index, mir_power_mode_on);
+                                   conf_output.preferred_mode_index, mir_pixel_format_xrgb_8888,
+                                   mir_power_mode_on);
         });
 
     display->configure(*conf);
@@ -508,7 +517,8 @@
         [&](mg::DisplayConfigurationOutput const& conf_output)
         {
             conf->configure_output(conf_output.id, false, conf_output.top_left,
-                                   conf_output.preferred_mode_index, mir_power_mode_on);
+                                   conf_output.preferred_mode_index, mir_pixel_format_xrgb_8888,
+                                   mir_power_mode_on);
         });
 
     display->configure(*conf);
 
=== modified file 'tests/unit-tests/graphics/mesa/test_overlapping_output_grouping.cpp'
--- tests/unit-tests/graphics/mesa/test_overlapping_output_grouping.cpp	2013-12-18 02:19:19 +0000
+++ tests/unit-tests/graphics/mesa/test_overlapping_output_grouping.cpp	2014-01-10 10:21:28 +0000
@@ -79,7 +79,7 @@
                 used,
                 rect.top_left,
                 i - 1,
-                0,
+                mir_pixel_format_invalid,
                 mir_power_mode_on
             };
 
@@ -89,7 +89,7 @@
     }
 
     void configure_output(mg::DisplayConfigurationOutputId, bool,
-                          geom::Point, size_t, MirPowerMode)
+                          geom::Point, size_t, MirPixelFormat, MirPowerMode) override
     {
     }
 
 
=== modified file 'tests/unit-tests/graphics/nested/test_nested_display_configuration.cpp'
--- tests/unit-tests/graphics/nested/test_nested_display_configuration.cpp	2013-09-12 21:36:55 +0000
+++ tests/unit-tests/graphics/nested/test_nested_display_configuration.cpp	2014-01-10 10:21:28 +0000
@@ -36,7 +36,7 @@
 uint32_t const default_current_mode = 0;
 uint32_t const default_num_output_formats = 0;
 MirPixelFormat* const default_output_formats = nullptr;
-uint32_t const default_current_output_format = 0;
+MirPixelFormat const default_current_output_format = mir_pixel_format_abgr_8888;
 uint32_t const default_card_id = 1;
 uint32_t const second_card_id = 2;
 uint32_t const default_output_id = 0;
@@ -84,7 +84,7 @@
         output->current_mode = 0;
         output->num_output_formats = NoOfFormats;
         output->output_formats = format_tmp;
-        output->current_output_format = 0;
+        output->current_format = NoOfFormats > 0 ? formats[0] : mir_pixel_format_invalid;
     }
 
     template<int NoOfOutputs, int NoOfModes, int NoOfFormats>
@@ -257,7 +257,7 @@
     mgn::NestedDisplayConfiguration config(build_trivial_configuration());
 
     config.configure_output(mg::DisplayConfigurationOutputId(default_output_id), true,
-        top_left, default_current_mode, mir_power_mode_on);
+        top_left, default_current_mode, default_current_output_format, mir_power_mode_on);
 
     MockOutputVisitor ov;
     EXPECT_CALL(ov, f(_)).Times(Exactly(1));
@@ -268,6 +268,7 @@
             EXPECT_EQ(true, output.used);
             EXPECT_EQ(top_left, output.top_left);
             EXPECT_EQ(0, output.current_mode_index);
+            EXPECT_EQ(default_current_output_format, output.current_format);
         });
 }
 
@@ -278,11 +279,25 @@
     mgn::NestedDisplayConfiguration config(build_trivial_configuration());
 
     EXPECT_THROW(
-        {config.configure_output(mg::DisplayConfigurationOutputId(default_output_id), true, top_left, -1, mir_power_mode_on);},
-        std::runtime_error);
-
-    EXPECT_THROW(
-        {config.configure_output(mg::DisplayConfigurationOutputId(default_output_id), true, top_left, too_big_mode_index, mir_power_mode_on);},
+        {config.configure_output(mg::DisplayConfigurationOutputId(default_output_id), true, top_left, -1, default_current_output_format, mir_power_mode_on);},
+        std::runtime_error);
+
+    EXPECT_THROW(
+        {config.configure_output(mg::DisplayConfigurationOutputId(default_output_id), true, top_left, too_big_mode_index, default_current_output_format, mir_power_mode_on);},
+        std::runtime_error);
+}
+
+TEST_F(NestedDisplayConfiguration, configure_output_rejects_invalid_format)
+{
+    geom::Point const top_left{10,20};
+    mgn::NestedDisplayConfiguration config(build_trivial_configuration());
+
+    EXPECT_THROW(
+        {config.configure_output(mg::DisplayConfigurationOutputId(default_output_id), true, top_left, default_current_mode, mir_pixel_format_invalid, mir_power_mode_on);},
+        std::runtime_error);
+
+    EXPECT_THROW(
+        {config.configure_output(mg::DisplayConfigurationOutputId(default_output_id), true, top_left, default_current_mode, mir_pixel_formats, mir_power_mode_on);},
         std::runtime_error);
 }
 
@@ -292,11 +307,11 @@
     mgn::NestedDisplayConfiguration config(build_trivial_configuration());
 
     EXPECT_THROW(
-        {config.configure_output(mg::DisplayConfigurationOutputId(default_output_id+1), true, top_left, default_current_mode, mir_power_mode_on);},
+        {config.configure_output(mg::DisplayConfigurationOutputId(default_output_id+1), true, top_left, default_current_mode, default_current_output_format, mir_power_mode_on);},
         std::runtime_error);
 
     EXPECT_THROW(
-        {config.configure_output(mg::DisplayConfigurationOutputId(default_output_id-1), true, top_left, default_current_mode, mir_power_mode_on);},
+        {config.configure_output(mg::DisplayConfigurationOutputId(default_output_id-1), true, top_left, default_current_mode, default_current_output_format, mir_power_mode_on);},
         std::runtime_error);
 }
 
@@ -326,7 +341,7 @@
     geom::Point const top_left{100,200};
     mgn::NestedDisplayConfiguration config(build_non_trivial_configuration());
 
-    config.configure_output(id, true, top_left, 1, mir_power_mode_on);
+    config.configure_output(id, true, top_left, 1, mir_pixel_format_argb_8888, mir_power_mode_on);
 
     MockOutputVisitor ov;
     EXPECT_CALL(ov, f(_)).Times(Exactly(3));
@@ -338,6 +353,7 @@
                 EXPECT_EQ(true, output.used);
                 EXPECT_EQ(top_left, output.top_left);
                 EXPECT_EQ(1, output.current_mode_index);
+                EXPECT_EQ(mir_pixel_format_argb_8888, output.current_format);
             }
         });
 }
 
=== modified file 'tests/unit-tests/graphics/test_default_display_configuration_policy.cpp'
--- tests/unit-tests/graphics/test_default_display_configuration_policy.cpp	2013-12-18 02:19:19 +0000
+++ tests/unit-tests/graphics/test_default_display_configuration_policy.cpp	2014-01-10 10:21:28 +0000
@@ -22,149 +22,158 @@
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
 
-namespace mg = mir::graphics;
-namespace geom = mir::geometry;
+using namespace mir::graphics;
+using namespace mir::geometry;
 
 namespace
 {
 
-class MockDisplayConfiguration : public mg::DisplayConfiguration
+class MockDisplayConfiguration : public DisplayConfiguration
 {
 public:
-    MockDisplayConfiguration(size_t max_simultaneous_outputs)
-        : card_id{1}, max_simultaneous_outputs{max_simultaneous_outputs}
+    MockDisplayConfiguration(MockDisplayConfiguration && m)
+        : max_simultaneous_outputs{m.max_simultaneous_outputs},
+        outputs{std::move(m.outputs)}
     {
-        /* Connected with modes */
-        outputs.push_back(
-            {
-                mg::DisplayConfigurationOutputId{10},
-                card_id,
-                mg::DisplayConfigurationOutputType::vga,
-                {
-                    mir_pixel_format_abgr_8888
-                },
-                {
-                    {geom::Size{123, 111}, 59.9},
-                    {geom::Size{123, 111}, 59.9},
-                    {geom::Size{123, 111}, 59.9}
-                },
-                2,
-                geom::Size{324, 642},
-                true,
-                false,
-                geom::Point{geom::X{123}, geom::Y{343}},
-                1,
-                0,
-                mir_power_mode_on
-            });
-        /* Connected without modes */
-        outputs.push_back(
-            {
-                mg::DisplayConfigurationOutputId{11},
-                card_id,
-                mg::DisplayConfigurationOutputType::vga,
-                {},
-                {},
-                std::numeric_limits<size_t>::max(),
-                geom::Size{566, 111},
-                true,
-                false,
-                geom::Point(),
-                std::numeric_limits<size_t>::max(),
-                std::numeric_limits<size_t>::max(),
-                mir_power_mode_on
-            });
-        /* Connected with a single mode */
-        outputs.push_back(
-            {
-                mg::DisplayConfigurationOutputId{12},
-                card_id,
-                mg::DisplayConfigurationOutputType::vga,
-                {
-                    mir_pixel_format_abgr_8888
-                },
-                {
-                    {geom::Size{523, 555}, 60.0},
-                },
-                0,
-                geom::Size{324, 642},
-                true,
-                false,
-                geom::Point(),
-                0,
-                0,
-                mir_power_mode_on
-            });
-        /* Not connected */
-        outputs.push_back(
-            {
-                mg::DisplayConfigurationOutputId{13},
-                card_id,
-                mg::DisplayConfigurationOutputType::vga,
-                {
-                    mir_pixel_format_abgr_8888
-                },
-                {},
-                0,
-                geom::Size{324, 642},
-                false,
-                false,
-                geom::Point(),
-                1,
-                0,
-                mir_power_mode_on
-            });
+    }
 
+    MockDisplayConfiguration(size_t max_simultaneous_outputs, std::vector<DisplayConfigurationOutput> && config)
+        : max_simultaneous_outputs{max_simultaneous_outputs},
+        outputs{config}
+    {
         if (max_simultaneous_outputs == max_simultaneous_outputs_all)
             max_simultaneous_outputs = outputs.size();
     }
 
-    MockDisplayConfiguration()
-        : MockDisplayConfiguration{max_simultaneous_outputs_all}
-    {
-    }
-
-    void for_each_card(std::function<void(mg::DisplayConfigurationCard const&)> f) const
-    {
-        f({card_id, max_simultaneous_outputs});
-    }
-
-    void for_each_output(std::function<void(mg::DisplayConfigurationOutput const&)> f) const
+    MockDisplayConfiguration(std::vector<DisplayConfigurationOutput> && config)
+        : MockDisplayConfiguration(max_simultaneous_outputs_all, std::move(config))
+    {
+    }
+
+    void for_each_card(std::function<void(DisplayConfigurationCard const&)> f) const
+    {
+        f({DisplayConfigurationCardId{1}, max_simultaneous_outputs});
+    }
+
+    void for_each_output(std::function<void(DisplayConfigurationOutput const&)> f) const
     {
         for (auto const& output : outputs)
             f(output);
     }
 
-    MOCK_METHOD5(configure_output, void(mg::DisplayConfigurationOutputId, bool,
-                                        geom::Point, size_t, MirPowerMode));
+    MOCK_METHOD6(configure_output, void(DisplayConfigurationOutputId, bool,
+                                        Point, size_t, MirPixelFormat, MirPowerMode));
 
+    static const size_t max_simultaneous_outputs_all{std::numeric_limits<size_t>::max()};
 private:
-    static const size_t max_simultaneous_outputs_all{std::numeric_limits<size_t>::max()};
-    mg::DisplayConfigurationCardId const card_id;
     size_t max_simultaneous_outputs;
-    std::vector<mg::DisplayConfigurationOutput> outputs;
+    std::vector<DisplayConfigurationOutput> outputs;
 };
 
 }
 
+DisplayConfigurationOutput default_output(DisplayConfigurationOutputId id)
+{
+    return { id, DisplayConfigurationCardId{1},
+        DisplayConfigurationOutputType::vga,
+        {mir_pixel_format_abgr_8888},
+        { {Size{523, 555}, 60.0} },
+        0,
+        Size{324, 642},
+        true,
+        false,
+        Point{X{123}, Y{343}},
+        0,
+        mir_pixel_format_abgr_8888,
+        mir_power_mode_on
+    };
+}
+
+DisplayConfigurationOutput connected_with_modes()
+{
+    DisplayConfigurationOutput output = default_output(DisplayConfigurationOutputId{10}) ;
+    output.modes =
+    {
+        {Size{123, 111}, 59.9},
+        {Size{123, 111}, 59.9},
+        {Size{123, 111}, 59.9}
+    };
+    output.preferred_mode_index = 2;
+    output.current_mode_index = 1;
+    return output;
+}
+DisplayConfigurationOutput connected_without_modes()
+{
+    DisplayConfigurationOutput output = default_output(DisplayConfigurationOutputId{11});
+    output.pixel_formats = {};
+    output.modes = {};
+    output.current_format = mir_pixel_format_invalid;
+    output.current_mode_index = std::numeric_limits<size_t>::max();
+    return output;
+}
+
+DisplayConfigurationOutput connected_with_single_mode()
+{
+    return default_output(DisplayConfigurationOutputId{12});
+}
+
+DisplayConfigurationOutput not_connected()
+{
+    DisplayConfigurationOutput output = default_output(DisplayConfigurationOutputId{13});
+    output.connected = false;
+    output.current_mode_index = 1;
+    return output;
+}
+
+DisplayConfigurationOutput connected_with_rgba_and_xrgb()
+{
+    DisplayConfigurationOutput output = default_output(DisplayConfigurationOutputId{14});
+    output.pixel_formats = {mir_pixel_format_argb_8888, mir_pixel_format_xrgb_8888};
+    return output;
+}
+
+DisplayConfigurationOutput connected_with_xrgb_bgr()
+{
+    DisplayConfigurationOutput output = default_output(DisplayConfigurationOutputId{15});
+    output.pixel_formats = {mir_pixel_format_xrgb_8888, mir_pixel_format_bgr_888};
+    output.current_format = mir_pixel_format_bgr_888;
+    return output;
+}
+
+MockDisplayConfiguration create_default_configuration(size_t max_outputs = MockDisplayConfiguration::max_simultaneous_outputs_all)
+{
+    return MockDisplayConfiguration
+    {
+        max_outputs,
+        {
+            connected_with_modes(),
+            connected_without_modes(),
+            connected_with_single_mode(),
+            not_connected(),
+        }
+    };
+}
+
 TEST(DefaultDisplayConfigurationPolicyTest, uses_all_connected_valid_outputs)
 {
     using namespace ::testing;
 
-    mg::DefaultDisplayConfigurationPolicy policy;
-    MockDisplayConfiguration conf;
+    DefaultDisplayConfigurationPolicy policy;
+    MockDisplayConfiguration conf{create_default_configuration()};
 
-    conf.for_each_output([&conf](mg::DisplayConfigurationOutput const& output)
+    conf.for_each_output([&conf](DisplayConfigurationOutput const& output)
     {
         if (output.connected && output.modes.size() > 0)
         {
-            EXPECT_CALL(conf, configure_output(output.id, true, geom::Point(),
-                                               output.preferred_mode_index, _));
+            EXPECT_CALL(conf, configure_output(output.id, true, Point(),
+                                               output.preferred_mode_index,
+                                               _, _));
         }
         else
         {
             EXPECT_CALL(conf, configure_output(output.id, false, output.top_left,
-                                               output.current_mode_index, _));
+                                               output.current_mode_index,
+                                               _, _));
         }
     });
 
@@ -175,12 +184,12 @@
 {
     using namespace ::testing;
 
-    mg::DefaultDisplayConfigurationPolicy policy;
-    MockDisplayConfiguration conf;
+    DefaultDisplayConfigurationPolicy policy;
+    MockDisplayConfiguration conf{create_default_configuration()};
 
-    conf.for_each_output([&conf](mg::DisplayConfigurationOutput const& output)
+    conf.for_each_output([&conf](DisplayConfigurationOutput const& output)
     {
-        EXPECT_CALL(conf, configure_output(output.id, _, _, _, mir_power_mode_on));
+        EXPECT_CALL(conf, configure_output(output.id, _, _, _, _, mir_power_mode_on));
     });
 
     policy.apply_to(conf);
@@ -191,20 +200,54 @@
     using namespace ::testing;
 
     size_t const max_simultaneous_outputs{1};
-    mg::DefaultDisplayConfigurationPolicy policy;
-    MockDisplayConfiguration conf{max_simultaneous_outputs};
+    DefaultDisplayConfigurationPolicy policy;
+    MockDisplayConfiguration conf{create_default_configuration(max_simultaneous_outputs)};
 
     size_t output_count{0};
-    conf.for_each_output([&output_count](mg::DisplayConfigurationOutput const&)
+    conf.for_each_output([&output_count](DisplayConfigurationOutput const&)
     {
         ++output_count;
     });
 
-    EXPECT_CALL(conf, configure_output(_, true, _, _, _))
+    EXPECT_CALL(conf, configure_output(_, true, _, _, _, _))
         .Times(AtMost(max_simultaneous_outputs));
 
-    EXPECT_CALL(conf, configure_output(_, false, _, _, _))
+    EXPECT_CALL(conf, configure_output(_, false, _, _, _, _))
         .Times(AtLeast(output_count - max_simultaneous_outputs));
 
     policy.apply_to(conf);
 }
+
+TEST(DefaultDisplayConfigurationPolicyTest, prefer_opaque_over_alpha)
+{
+    using namespace ::testing;
+
+    DefaultDisplayConfigurationPolicy policy;
+    MockDisplayConfiguration pick_xrgb{ { connected_with_rgba_and_xrgb() } };
+
+    EXPECT_CALL(pick_xrgb, configure_output(_, true, _, _, mir_pixel_format_xrgb_8888, _));
+    policy.apply_to(pick_xrgb);
+}
+
+TEST(DefaultDisplayConfigurationPolicyTest, preserve_opaque_selection)
+{
+    using namespace ::testing;
+
+    DefaultDisplayConfigurationPolicy policy;
+    MockDisplayConfiguration keep_bgr{ { connected_with_xrgb_bgr() } };
+
+    EXPECT_CALL(keep_bgr, configure_output(_, true, _, _, mir_pixel_format_bgr_888, _));
+    policy.apply_to(keep_bgr);
+}
+
+TEST(DefaultDisplayConfigurationPolicyTest, accept_transparency_when_only_option)
+{
+    using namespace ::testing;
+
+    DefaultDisplayConfigurationPolicy policy;
+    MockDisplayConfiguration pick_rgba{ { default_output(DisplayConfigurationOutputId{15}) } };
+
+    EXPECT_CALL(pick_rgba, configure_output(_, true, _, _, mir_pixel_format_abgr_8888, _));
+    policy.apply_to(pick_rgba);
+}
+
 
=== modified file 'tests/unit-tests/graphics/test_display_configuration.cpp'
--- tests/unit-tests/graphics/test_display_configuration.cpp	2013-12-18 02:19:19 +0000
+++ tests/unit-tests/graphics/test_display_configuration.cpp	2014-01-10 10:21:28 +0000
@@ -45,7 +45,7 @@
     true,
     geom::Point(),
     2,
-    0,
+    mir_pixel_format_abgr_8888,
     mir_power_mode_on
 };