Mir

Merge lp:~andreas-pokorny/mir/pixel-format-utils into lp:mir

Proposed by Andreas Pokorny
Status: Merged
Approved by: kevin gunn
Approved revision: no longer in the source branch.
Merged at revision: 1324
Proposed branch: lp:~andreas-pokorny/mir/pixel-format-utils
Merge into: lp:mir
Diff against target: 238 lines (+202/-0)
5 files modified
include/platform/mir/graphics/pixel_format_utils.h (+50/-0)
src/platform/graphics/CMakeLists.txt (+1/-0)
src/platform/graphics/pixel_format_utils.cpp (+54/-0)
tests/unit-tests/graphics/CMakeLists.txt (+1/-0)
tests/unit-tests/graphics/test_pixel_format_utils.cpp (+96/-0)
To merge this branch: bzr merge lp:~andreas-pokorny/mir/pixel-format-utils
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Alexandros Frantzis (community) Approve
Daniel van Vugt Approve
Kevin DuBois (community) Approve
Alan Griffiths Approve
Robert Carr (community) Approve
Andreas Pokorny (community) Approve
Review via email: mp+200278@code.launchpad.net

This proposal supersedes a proposal from 2014-01-02.

Commit message

Adds mir pixel format utility functions to query details about MirPixelFormat

Description of the change

Add mir pixel format utility functions

To post a comment you must log in.
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

No major problems, but there are enough minor issues to justify a "Needs Fixing".

(1) The copyright year is now wrong :)

(2) Using the strongly typed MirPixelFormat, you don't have to check for invalid types. So:
100 + if (mir_pixel_format_invalid < format &&
101 + format < mir_pixel_formats)
102 + return 8;
103 + return 0;
could just be:
    return 8;
This is because it's already a precondition that "format" is a valid MirPixelFormat. The same goes for other functions. You never need to range-check a strongly typed enum.

(3) I assume the valid_format() function is for testing non-MirPixelFormat variables; integers? As such it might make sense for:
    bool valid_format(MirPixelFormat format)
to be:
    bool valid_mir_pixel_format(int format)
?

(4) TEST(MirPixelformat, ...
It is more correct and still safe to use the correct capitalization "MirPixelFormat". Although calling the test "PixelFormatUtils" might be even more accurate.

review: Needs Fixing
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

> No major problems, but there are enough minor issues to justify a "Needs
> Fixing".
>
> (1) The copyright year is now wrong :)

Those years are updated when a file is touched?

> (2) Using the strongly typed MirPixelFormat, you don't have to check for
> invalid types. So:
> 100 + if (mir_pixel_format_invalid < format &&
> 101 + format < mir_pixel_formats)
> 102 + return 8;
> 103 + return 0;
> could just be:
> return 8;
> This is because it's already a precondition that "format" is a valid
> MirPixelFormat. The same goes for other functions. You never need to range-
> check a strongly typed enum.

But it is not really strongly typed - an unchecked static_cast is enough to turn an arbitrary integer into a meaningless pixel format. Additionally the enum itself provides invalid values too.

>
> (3) I assume the valid_format() function is for testing non-MirPixelFormat
> variables; integers? As such it might make sense for:
> bool valid_format(MirPixelFormat format)
> to be:
> bool valid_mir_pixel_format(int format)
> ?

At the point where it is used the integers transfered to the client have already been "converted". But the new name a a good idea/

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

(2) It's a precondition. If the parameter is a MirPixelFormat then you are guaranteed that the value is a valid MirPixelFormat. If that's not yet guaranteed the please ensure it is. Otherwise the code would be littered with countless range checks every time we use an enum for anything :)

Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

missing include guard..

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

include guard fixed

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Robert Carr (robertcarr) wrote :

LGTM.

"valid_mir_pixel_format(MirPixelFormat..."

Since "Mir" is encoded in the type name, I think "don't repeat yourself" suggests just 'valid_pixel_format'. No block though.

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

(4) It's dangerous to assume mir_pixel_format_invalid has a lower integer value than the valid pixel formats:
   mir_pixel_format_invalid < format
unless the order is somehow guaranteed by the name. The same is possibly also true about "mir_pixel_formats" if and when we choose to encode more information into the mir_pixel_format_ values.

You can make such assumptions more safely if it's defined as a macro adjacent to the definition of MirPixelFormat. Then you can see both definitions side-by-side and the risk of one regressing independently of the other is much lower.

review: Needs Fixing
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

> (4) It's dangerous to assume mir_pixel_format_invalid has a lower integer
> value than the valid pixel formats:
> mir_pixel_format_invalid < format
> unless the order is somehow guaranteed by the name. The same is possibly also
> true about "mir_pixel_formats" if and when we choose to encode more
> information into the mir_pixel_format_ values.
>
> You can make such assumptions more safely if it's defined as a macro adjacent
> to the definition of MirPixelFormat. Then you can see both definitions side-
> by-side and the risk of one regressing independently of the other is much
> lower.

As soon as somebody changes the enumeration - like reordering - the tests will probably fail and a better implementation is necessary. Actually the current implementation is just the result of doing tdd up to the point where those tests run. I just assumed that nobody will make changes that are not driven by tests.

I would like to redesign the enumeration and add support for more formats, but as you already said in a previous review of the same code that can be added later.

Maybe it is better to make bigger review..

Revision history for this message
Kevin DuBois (kdub) wrote :

This would have been something nice to have put in mir::geometry::PixelFormat as more c++ functions on a class instead of boiling everything down to c-style enums with freestanding functions.

Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

Ok

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Please undo class PixelFormat. It took a lot of effort [1] to de-duplicate pixel format types.

Also, the functions will at some stage likely be replaced by macros (LP: #1236254) so simple functions are a better stepping-stone.

[1] https://code.launchpad.net/~vanvugt/mir/one-PixelFormat-type/+merge/196852

review: Needs Fixing
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

> Please undo class PixelFormat. It took a lot of effort [1] to de-duplicate
> pixel format types.
>
> Also, the functions will at some stage likely be replaced by macros (LP:
> #1236254) so simple functions are a better stepping-stone.
>
> [1] https://code.launchpad.net/~vanvugt/mir/one-PixelFormat-type/+merge/196852

I was not aware of the similarities between the MP and previously existing code.
The current MP does not introduce a duplicate type - just a wrapper to make code
less verbose in some reasonable cases.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

105 + return (format > mir_pixel_format_invalid &&
106 + format < mir_pixel_formats);

If we are going to range-check the enums, it would be good to have a test for the upper edge too:

EXPECT_FALSE(valid_pixel_format(mir_pixels_formats));

169 +#include <gmock/gmock.h>

Not needed.

92 +namespace mir
93 +{
94 +namespace graphics
95 +{

Have we decided to allow this scheme in implementation files (vs "using mg=...; mg::...") ?

review: Needs Fixing
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> 92 +namespace mir
> 93 +{
> 94 +namespace graphics
> 95 +{
>
> Have we decided to allow this scheme in implementation files (vs "using
> mg=...; mg::...") ?

It isn't banned by the style guide, but I don't like it either.

There is a little existing code in this style:

$ grep "namespace" `find src -name \*.cpp` | grep -v = | grep -v using | grep -v namespace$
src/shared/protobuf/google_protobuf_guard.cpp:namespace mir
src/server/compositor/multi_threaded_compositor.cpp:namespace mir
src/server/compositor/multi_threaded_compositor.cpp:namespace compositor
src/server/scene/threaded_snapshot_strategy.cpp:namespace mir
src/server/scene/threaded_snapshot_strategy.cpp:namespace scene

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

6 + * Copyright © 2013 Canonical Ltd.

Judging by commit dates this is wrong

review: Needs Fixing
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

> > 92 +namespace mir
> > 93 +{
> > 94 +namespace graphics
> > 95 +{
> >
> > Have we decided to allow this scheme in implementation files (vs "using
> > mg=...; mg::...") ?
>
> It isn't banned by the style guide, but I don't like it either.

And you convinced me. I missed that one.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

LGTM

review: Approve
Revision history for this message
Kevin DuBois (kdub) wrote :

I still have reservations that "deduplicating the pixel format types" was the right thing to do. We had a c-compatible pixel format and an internal pixel format class whose intention was to be more versatile and have functions on the formats to check the alpha capabilities, color depths, etc.
However, those reservations shouldn't block this review. Looks okay to me

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

(4) It's still dangerous to assume mir_pixel_format_invalid is lower than valid formats;
102 + return (format > mir_pixel_format_invalid &&

I think this is the one remaining concern raised by both Alexandros and myself. I would say "Needs fixing", but it's minor and you've suffered enough.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

163 +#include <gmock/gmock.h>

Still not needed, but not critical.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== added file 'include/platform/mir/graphics/pixel_format_utils.h'
--- include/platform/mir/graphics/pixel_format_utils.h 1970-01-01 00:00:00 +0000
+++ include/platform/mir/graphics/pixel_format_utils.h 2014-01-08 16:08:40 +0000
@@ -0,0 +1,50 @@
1/*
2 * Copyright © 2014 Canonical Ltd.
3 *
4 * This program is free software: you can redistribute it and/or modify it
5 * under the terms of the GNU Lesser General Public License version 3,
6 * as published by the Free Software Foundation.
7 *
8 * This program is distributed in the hope that it will be useful,
9 * but WITHOUT ANY WARRANTY; without even the implied warranty of
10 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11 * GNU Lesser General Public License for more details.
12 *
13 * You should have received a copy of the GNU Lesser General Public License
14 * along with this program. If not, see <http://www.gnu.org/licenses/>.
15 *
16 * Authored by: Andreas Pokorny <andreas.pokorny@canonical.com>
17 */
18
19#ifndef MIR_GRAPHICS_PIXEL_FORMAT_UTILS_H_
20#define MIR_GRAPHICS_PIXEL_FORMAT_UTILS_H_
21
22#include "mir_toolkit/common.h"
23
24namespace mir
25{
26namespace graphics
27{
28
29/*!
30 * \name MirPixelFormat utility functions
31 *
32 * A set of functions to query details of MirPixelFormat
33 * TODO improve this through https://bugs.launchpad.net/mir/+bug/1236254
34 * \{
35 */
36bool contains_alpha(MirPixelFormat format);
37int red_channel_depth(MirPixelFormat format);
38int blue_channel_depth(MirPixelFormat format);
39int green_channel_depth(MirPixelFormat format);
40int alpha_channel_depth(MirPixelFormat format);
41bool valid_pixel_format(MirPixelFormat format);
42/*!
43 * \}
44 */
45
46
47}
48}
49
50#endif
051
=== modified file 'src/platform/graphics/CMakeLists.txt'
--- src/platform/graphics/CMakeLists.txt 2013-12-18 02:19:19 +0000
+++ src/platform/graphics/CMakeLists.txt 2014-01-08 16:08:40 +0000
@@ -10,6 +10,7 @@
10 display_configuration.cpp10 display_configuration.cpp
11 null_display_report.cpp11 null_display_report.cpp
12 buffer_basic.cpp12 buffer_basic.cpp
13 pixel_format_utils.cpp
13)14)
1415
15add_library(16add_library(
1617
=== added file 'src/platform/graphics/pixel_format_utils.cpp'
--- src/platform/graphics/pixel_format_utils.cpp 1970-01-01 00:00:00 +0000
+++ src/platform/graphics/pixel_format_utils.cpp 2014-01-08 16:08:40 +0000
@@ -0,0 +1,54 @@
1/*
2 * Copyright © 2014 Canonical Ltd.
3 *
4 * This program is free software: you can redistribute it and/or modify it
5 * under the terms of the GNU Lesser General Public License version 3,
6 * as published by the Free Software Foundation.
7 *
8 * This program is distributed in the hope that it will be useful,
9 * but WITHOUT ANY WARRANTY; without even the implied warranty of
10 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11 * GNU Lesser General Public License for more details.
12 *
13 * You should have received a copy of the GNU Lesser General Public License
14 * along with this program. If not, see <http://www.gnu.org/licenses/>.
15 *
16 * Authored by: Andreas Pokorny <andreas.pokorny@canonical.com>
17 */
18
19#include "mir/graphics/pixel_format_utils.h"
20
21namespace mg = mir::graphics;
22
23bool mg::contains_alpha(MirPixelFormat format)
24{
25 return (format == mir_pixel_format_abgr_8888 ||
26 format == mir_pixel_format_argb_8888);
27}
28
29bool mg::valid_pixel_format(MirPixelFormat format)
30{
31 return (format > mir_pixel_format_invalid &&
32 format < mir_pixel_formats);
33}
34
35int mg::red_channel_depth(MirPixelFormat format)
36{
37 return valid_pixel_format(format) ? 8 : 0;
38}
39
40int mg::blue_channel_depth(MirPixelFormat format)
41{
42 return valid_pixel_format(format) ? 8 : 0;
43}
44
45int mg::green_channel_depth(MirPixelFormat format)
46{
47 return valid_pixel_format(format) ? 8 : 0;
48}
49
50int mg::alpha_channel_depth(MirPixelFormat format)
51{
52 return contains_alpha(format) ? 8 : 0;
53}
54
055
=== modified file 'tests/unit-tests/graphics/CMakeLists.txt'
--- tests/unit-tests/graphics/CMakeLists.txt 2013-12-20 05:06:28 +0000
+++ tests/unit-tests/graphics/CMakeLists.txt 2014-01-08 16:08:40 +0000
@@ -6,6 +6,7 @@
6 ${CMAKE_CURRENT_SOURCE_DIR}/test_default_display_configuration_policy.cpp6 ${CMAKE_CURRENT_SOURCE_DIR}/test_default_display_configuration_policy.cpp
7 ${CMAKE_CURRENT_SOURCE_DIR}/test_buffer_id.cpp7 ${CMAKE_CURRENT_SOURCE_DIR}/test_buffer_id.cpp
8 ${CMAKE_CURRENT_SOURCE_DIR}/test_buffer_properties.cpp8 ${CMAKE_CURRENT_SOURCE_DIR}/test_buffer_properties.cpp
9 ${CMAKE_CURRENT_SOURCE_DIR}/test_pixel_format_utils.cpp
9 ${CMAKE_CURRENT_SOURCE_DIR}/test_surfaceless_egl_context.cpp10 ${CMAKE_CURRENT_SOURCE_DIR}/test_surfaceless_egl_context.cpp
10)11)
1112
1213
=== added file 'tests/unit-tests/graphics/test_pixel_format_utils.cpp'
--- tests/unit-tests/graphics/test_pixel_format_utils.cpp 1970-01-01 00:00:00 +0000
+++ tests/unit-tests/graphics/test_pixel_format_utils.cpp 2014-01-08 16:08:40 +0000
@@ -0,0 +1,96 @@
1/*
2 * Copyright © 2014 Canonical Ltd.
3 *
4 * This program is free software: you can redistribute it and/or modify it
5 * under the terms of the GNU General Public License version 3,
6 * as published by the Free Software Foundation.
7 *
8 * This program is distributed in the hope that it will be useful,
9 * but WITHOUT ANY WARRANTY; without even the implied warranty of
10 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11 * GNU General Public License for more details.
12 *
13 * You should have received a copy of the GNU General Public License
14 * along with this program. If not, see <http://www.gnu.org/licenses/>.
15 *
16 * Authored by: Andreas Pokorny <andreas.pokorny@canonical.com>
17 */
18
19#include "mir_toolkit/common.h"
20#include "mir/graphics/pixel_format_utils.h"
21#include <gmock/gmock.h>
22#include <gtest/gtest.h>
23
24using namespace mir::graphics;
25TEST(MirPixelFormatUtils, contains_alpha)
26{
27 EXPECT_FALSE(contains_alpha(mir_pixel_format_xbgr_8888));
28 EXPECT_FALSE(contains_alpha(mir_pixel_format_bgr_888));
29 EXPECT_FALSE(contains_alpha(mir_pixel_format_xrgb_8888));
30 EXPECT_FALSE(contains_alpha(mir_pixel_format_xbgr_8888));
31 EXPECT_TRUE(contains_alpha(mir_pixel_format_argb_8888));
32 EXPECT_TRUE(contains_alpha(mir_pixel_format_abgr_8888));
33 EXPECT_FALSE(contains_alpha(mir_pixel_format_invalid));
34 EXPECT_FALSE(contains_alpha(mir_pixel_formats));
35}
36
37TEST(MirPixelFormatUtils, red_channel_depths)
38{
39 EXPECT_EQ(8, red_channel_depth(mir_pixel_format_xbgr_8888));
40 EXPECT_EQ(8, red_channel_depth(mir_pixel_format_bgr_888));
41 EXPECT_EQ(8, red_channel_depth(mir_pixel_format_xrgb_8888));
42 EXPECT_EQ(8, red_channel_depth(mir_pixel_format_xbgr_8888));
43 EXPECT_EQ(8, red_channel_depth(mir_pixel_format_argb_8888));
44 EXPECT_EQ(8, red_channel_depth(mir_pixel_format_abgr_8888));
45 EXPECT_EQ(0, red_channel_depth(mir_pixel_format_invalid));
46 EXPECT_EQ(0, red_channel_depth(mir_pixel_formats));
47}
48
49TEST(MirPixelFormatUtils, blue_channel_depths)
50{
51 EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_xbgr_8888));
52 EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_bgr_888));
53 EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_xrgb_8888));
54 EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_xbgr_8888));
55 EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_argb_8888));
56 EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_abgr_8888));
57 EXPECT_EQ(0, blue_channel_depth(mir_pixel_format_invalid));
58 EXPECT_EQ(0, blue_channel_depth(mir_pixel_formats));
59}
60
61TEST(MirPixelFormatUtils, green_channel_depths)
62{
63 EXPECT_EQ(8, green_channel_depth(mir_pixel_format_xbgr_8888));
64 EXPECT_EQ(8, green_channel_depth(mir_pixel_format_bgr_888));
65 EXPECT_EQ(8, green_channel_depth(mir_pixel_format_xrgb_8888));
66 EXPECT_EQ(8, green_channel_depth(mir_pixel_format_xbgr_8888));
67 EXPECT_EQ(8, green_channel_depth(mir_pixel_format_argb_8888));
68 EXPECT_EQ(8, green_channel_depth(mir_pixel_format_abgr_8888));
69 EXPECT_EQ(0, green_channel_depth(mir_pixel_format_invalid));
70 EXPECT_EQ(0, green_channel_depth(mir_pixel_formats));
71}
72
73
74TEST(MirPixelFormatUtils, alpha_channel_depths)
75{
76 EXPECT_EQ(0, alpha_channel_depth(mir_pixel_format_xbgr_8888));
77 EXPECT_EQ(0, alpha_channel_depth(mir_pixel_format_bgr_888));
78 EXPECT_EQ(0, alpha_channel_depth(mir_pixel_format_xrgb_8888));
79 EXPECT_EQ(0, alpha_channel_depth(mir_pixel_format_xbgr_8888));
80 EXPECT_EQ(8, alpha_channel_depth(mir_pixel_format_argb_8888));
81 EXPECT_EQ(8, alpha_channel_depth(mir_pixel_format_abgr_8888));
82 EXPECT_EQ(0, alpha_channel_depth(mir_pixel_format_invalid));
83 EXPECT_EQ(0, alpha_channel_depth(mir_pixel_formats));
84}
85
86TEST(MirPixelFormatUtils, valid_mir_pixel_format)
87{
88 EXPECT_TRUE(valid_pixel_format(mir_pixel_format_xbgr_8888));
89 EXPECT_TRUE(valid_pixel_format(mir_pixel_format_bgr_888));
90 EXPECT_TRUE(valid_pixel_format(mir_pixel_format_xrgb_8888));
91 EXPECT_TRUE(valid_pixel_format(mir_pixel_format_xbgr_8888));
92 EXPECT_TRUE(valid_pixel_format(mir_pixel_format_argb_8888));
93 EXPECT_TRUE(valid_pixel_format(mir_pixel_format_abgr_8888));
94 EXPECT_FALSE(valid_pixel_format(mir_pixel_format_invalid));
95 EXPECT_FALSE(valid_pixel_format(mir_pixel_formats));
96}

Subscribers

People subscribed via source and target branches