Mir

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

Proposed by Andreas Pokorny on 2014-01-02
Status: Merged
Approved by: kevin gunn on 2014-01-09
Approved revision: 1309
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 on 2014-01-09
Alexandros Frantzis (community) Approve on 2014-01-09
Daniel van Vugt 2014-01-02 Approve on 2014-01-09
Kevin DuBois (community) Approve on 2014-01-09
Alan Griffiths Approve on 2014-01-08
Robert Carr (community) Approve on 2014-01-02
Andreas Pokorny (community) Approve on 2014-01-02
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.
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
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/

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

Andreas Pokorny (andreas-pokorny) wrote :

missing include guard..

review: Needs Fixing
Andreas Pokorny (andreas-pokorny) wrote :

include guard fixed

review: Approve
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
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
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..

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.

Andreas Pokorny (andreas-pokorny) wrote :

Ok

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

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

Alan Griffiths (alan-griffiths) wrote :

6 + * Copyright © 2013 Canonical Ltd.

Judging by commit dates this is wrong

review: Needs Fixing
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.

Alan Griffiths (alan-griffiths) wrote :

LGTM

review: Approve
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
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
Alexandros Frantzis (afrantzis) wrote :

163 +#include <gmock/gmock.h>

Still not needed, but not critical.

review: Approve
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'include/platform/mir/graphics/pixel_format_utils.h'
2--- include/platform/mir/graphics/pixel_format_utils.h 1970-01-01 00:00:00 +0000
3+++ include/platform/mir/graphics/pixel_format_utils.h 2014-01-08 16:08:40 +0000
4@@ -0,0 +1,50 @@
5+/*
6+ * Copyright © 2014 Canonical Ltd.
7+ *
8+ * This program is free software: you can redistribute it and/or modify it
9+ * under the terms of the GNU Lesser General Public License version 3,
10+ * as published by the Free Software Foundation.
11+ *
12+ * This program is distributed in the hope that it will be useful,
13+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
14+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
15+ * GNU Lesser General Public License for more details.
16+ *
17+ * You should have received a copy of the GNU Lesser General Public License
18+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
19+ *
20+ * Authored by: Andreas Pokorny <andreas.pokorny@canonical.com>
21+ */
22+
23+#ifndef MIR_GRAPHICS_PIXEL_FORMAT_UTILS_H_
24+#define MIR_GRAPHICS_PIXEL_FORMAT_UTILS_H_
25+
26+#include "mir_toolkit/common.h"
27+
28+namespace mir
29+{
30+namespace graphics
31+{
32+
33+/*!
34+ * \name MirPixelFormat utility functions
35+ *
36+ * A set of functions to query details of MirPixelFormat
37+ * TODO improve this through https://bugs.launchpad.net/mir/+bug/1236254
38+ * \{
39+ */
40+bool contains_alpha(MirPixelFormat format);
41+int red_channel_depth(MirPixelFormat format);
42+int blue_channel_depth(MirPixelFormat format);
43+int green_channel_depth(MirPixelFormat format);
44+int alpha_channel_depth(MirPixelFormat format);
45+bool valid_pixel_format(MirPixelFormat format);
46+/*!
47+ * \}
48+ */
49+
50+
51+}
52+}
53+
54+#endif
55
56=== modified file 'src/platform/graphics/CMakeLists.txt'
57--- src/platform/graphics/CMakeLists.txt 2013-12-18 02:19:19 +0000
58+++ src/platform/graphics/CMakeLists.txt 2014-01-08 16:08:40 +0000
59@@ -10,6 +10,7 @@
60 display_configuration.cpp
61 null_display_report.cpp
62 buffer_basic.cpp
63+ pixel_format_utils.cpp
64 )
65
66 add_library(
67
68=== added file 'src/platform/graphics/pixel_format_utils.cpp'
69--- src/platform/graphics/pixel_format_utils.cpp 1970-01-01 00:00:00 +0000
70+++ src/platform/graphics/pixel_format_utils.cpp 2014-01-08 16:08:40 +0000
71@@ -0,0 +1,54 @@
72+/*
73+ * Copyright © 2014 Canonical Ltd.
74+ *
75+ * This program is free software: you can redistribute it and/or modify it
76+ * under the terms of the GNU Lesser General Public License version 3,
77+ * as published by the Free Software Foundation.
78+ *
79+ * This program is distributed in the hope that it will be useful,
80+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
81+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
82+ * GNU Lesser General Public License for more details.
83+ *
84+ * You should have received a copy of the GNU Lesser General Public License
85+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
86+ *
87+ * Authored by: Andreas Pokorny <andreas.pokorny@canonical.com>
88+ */
89+
90+#include "mir/graphics/pixel_format_utils.h"
91+
92+namespace mg = mir::graphics;
93+
94+bool mg::contains_alpha(MirPixelFormat format)
95+{
96+ return (format == mir_pixel_format_abgr_8888 ||
97+ format == mir_pixel_format_argb_8888);
98+}
99+
100+bool mg::valid_pixel_format(MirPixelFormat format)
101+{
102+ return (format > mir_pixel_format_invalid &&
103+ format < mir_pixel_formats);
104+}
105+
106+int mg::red_channel_depth(MirPixelFormat format)
107+{
108+ return valid_pixel_format(format) ? 8 : 0;
109+}
110+
111+int mg::blue_channel_depth(MirPixelFormat format)
112+{
113+ return valid_pixel_format(format) ? 8 : 0;
114+}
115+
116+int mg::green_channel_depth(MirPixelFormat format)
117+{
118+ return valid_pixel_format(format) ? 8 : 0;
119+}
120+
121+int mg::alpha_channel_depth(MirPixelFormat format)
122+{
123+ return contains_alpha(format) ? 8 : 0;
124+}
125+
126
127=== modified file 'tests/unit-tests/graphics/CMakeLists.txt'
128--- tests/unit-tests/graphics/CMakeLists.txt 2013-12-20 05:06:28 +0000
129+++ tests/unit-tests/graphics/CMakeLists.txt 2014-01-08 16:08:40 +0000
130@@ -6,6 +6,7 @@
131 ${CMAKE_CURRENT_SOURCE_DIR}/test_default_display_configuration_policy.cpp
132 ${CMAKE_CURRENT_SOURCE_DIR}/test_buffer_id.cpp
133 ${CMAKE_CURRENT_SOURCE_DIR}/test_buffer_properties.cpp
134+ ${CMAKE_CURRENT_SOURCE_DIR}/test_pixel_format_utils.cpp
135 ${CMAKE_CURRENT_SOURCE_DIR}/test_surfaceless_egl_context.cpp
136 )
137
138
139=== added file 'tests/unit-tests/graphics/test_pixel_format_utils.cpp'
140--- tests/unit-tests/graphics/test_pixel_format_utils.cpp 1970-01-01 00:00:00 +0000
141+++ tests/unit-tests/graphics/test_pixel_format_utils.cpp 2014-01-08 16:08:40 +0000
142@@ -0,0 +1,96 @@
143+/*
144+ * Copyright © 2014 Canonical Ltd.
145+ *
146+ * This program is free software: you can redistribute it and/or modify it
147+ * under the terms of the GNU General Public License version 3,
148+ * as published by the Free Software Foundation.
149+ *
150+ * This program is distributed in the hope that it will be useful,
151+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
152+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
153+ * GNU General Public License for more details.
154+ *
155+ * You should have received a copy of the GNU General Public License
156+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
157+ *
158+ * Authored by: Andreas Pokorny <andreas.pokorny@canonical.com>
159+ */
160+
161+#include "mir_toolkit/common.h"
162+#include "mir/graphics/pixel_format_utils.h"
163+#include <gmock/gmock.h>
164+#include <gtest/gtest.h>
165+
166+using namespace mir::graphics;
167+TEST(MirPixelFormatUtils, contains_alpha)
168+{
169+ EXPECT_FALSE(contains_alpha(mir_pixel_format_xbgr_8888));
170+ EXPECT_FALSE(contains_alpha(mir_pixel_format_bgr_888));
171+ EXPECT_FALSE(contains_alpha(mir_pixel_format_xrgb_8888));
172+ EXPECT_FALSE(contains_alpha(mir_pixel_format_xbgr_8888));
173+ EXPECT_TRUE(contains_alpha(mir_pixel_format_argb_8888));
174+ EXPECT_TRUE(contains_alpha(mir_pixel_format_abgr_8888));
175+ EXPECT_FALSE(contains_alpha(mir_pixel_format_invalid));
176+ EXPECT_FALSE(contains_alpha(mir_pixel_formats));
177+}
178+
179+TEST(MirPixelFormatUtils, red_channel_depths)
180+{
181+ EXPECT_EQ(8, red_channel_depth(mir_pixel_format_xbgr_8888));
182+ EXPECT_EQ(8, red_channel_depth(mir_pixel_format_bgr_888));
183+ EXPECT_EQ(8, red_channel_depth(mir_pixel_format_xrgb_8888));
184+ EXPECT_EQ(8, red_channel_depth(mir_pixel_format_xbgr_8888));
185+ EXPECT_EQ(8, red_channel_depth(mir_pixel_format_argb_8888));
186+ EXPECT_EQ(8, red_channel_depth(mir_pixel_format_abgr_8888));
187+ EXPECT_EQ(0, red_channel_depth(mir_pixel_format_invalid));
188+ EXPECT_EQ(0, red_channel_depth(mir_pixel_formats));
189+}
190+
191+TEST(MirPixelFormatUtils, blue_channel_depths)
192+{
193+ EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_xbgr_8888));
194+ EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_bgr_888));
195+ EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_xrgb_8888));
196+ EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_xbgr_8888));
197+ EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_argb_8888));
198+ EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_abgr_8888));
199+ EXPECT_EQ(0, blue_channel_depth(mir_pixel_format_invalid));
200+ EXPECT_EQ(0, blue_channel_depth(mir_pixel_formats));
201+}
202+
203+TEST(MirPixelFormatUtils, green_channel_depths)
204+{
205+ EXPECT_EQ(8, green_channel_depth(mir_pixel_format_xbgr_8888));
206+ EXPECT_EQ(8, green_channel_depth(mir_pixel_format_bgr_888));
207+ EXPECT_EQ(8, green_channel_depth(mir_pixel_format_xrgb_8888));
208+ EXPECT_EQ(8, green_channel_depth(mir_pixel_format_xbgr_8888));
209+ EXPECT_EQ(8, green_channel_depth(mir_pixel_format_argb_8888));
210+ EXPECT_EQ(8, green_channel_depth(mir_pixel_format_abgr_8888));
211+ EXPECT_EQ(0, green_channel_depth(mir_pixel_format_invalid));
212+ EXPECT_EQ(0, green_channel_depth(mir_pixel_formats));
213+}
214+
215+
216+TEST(MirPixelFormatUtils, alpha_channel_depths)
217+{
218+ EXPECT_EQ(0, alpha_channel_depth(mir_pixel_format_xbgr_8888));
219+ EXPECT_EQ(0, alpha_channel_depth(mir_pixel_format_bgr_888));
220+ EXPECT_EQ(0, alpha_channel_depth(mir_pixel_format_xrgb_8888));
221+ EXPECT_EQ(0, alpha_channel_depth(mir_pixel_format_xbgr_8888));
222+ EXPECT_EQ(8, alpha_channel_depth(mir_pixel_format_argb_8888));
223+ EXPECT_EQ(8, alpha_channel_depth(mir_pixel_format_abgr_8888));
224+ EXPECT_EQ(0, alpha_channel_depth(mir_pixel_format_invalid));
225+ EXPECT_EQ(0, alpha_channel_depth(mir_pixel_formats));
226+}
227+
228+TEST(MirPixelFormatUtils, valid_mir_pixel_format)
229+{
230+ EXPECT_TRUE(valid_pixel_format(mir_pixel_format_xbgr_8888));
231+ EXPECT_TRUE(valid_pixel_format(mir_pixel_format_bgr_888));
232+ EXPECT_TRUE(valid_pixel_format(mir_pixel_format_xrgb_8888));
233+ EXPECT_TRUE(valid_pixel_format(mir_pixel_format_xbgr_8888));
234+ EXPECT_TRUE(valid_pixel_format(mir_pixel_format_argb_8888));
235+ EXPECT_TRUE(valid_pixel_format(mir_pixel_format_abgr_8888));
236+ EXPECT_FALSE(valid_pixel_format(mir_pixel_format_invalid));
237+ EXPECT_FALSE(valid_pixel_format(mir_pixel_formats));
238+}

Subscribers

People subscribed via source and target branches