Merge lp:~andreas-pokorny/mir/pixel-format-utils into lp:mir
- pixel-format-utils
- Merge into development-branch
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 |
Related bugs: |
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
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_
> 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(
> to be:
> bool valid_mir_
> ?
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..
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1304
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Andreas Pokorny (andreas-pokorny) wrote : | # |
include guard fixed
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1306
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Robert Carr (robertcarr) wrote : | # |
LGTM.
"valid_
Since "Mir" is encoded in the type name, I think "don't repeat yourself" suggests just 'valid_
Daniel van Vugt (vanvugt) wrote : | # |
(4) It's dangerous to assume mir_pixel_
mir_
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.
Andreas Pokorny (andreas-pokorny) wrote : | # |
> (4) It's dangerous to assume mir_pixel_
> value than the valid pixel formats:
> mir_pixel_
> 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:
Andreas Pokorny (andreas-pokorny) wrote : | # |
Ok
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1307
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
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:/
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:/
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.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1308
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Alexandros Frantzis (afrantzis) wrote : | # |
105 + return (format > mir_pixel_
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_
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::...") ?
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/
src/server/
src/server/
src/server/
src/server/
Alan Griffiths (alan-griffiths) wrote : | # |
6 + * Copyright © 2013 Canonical Ltd.
Judging by commit dates this is wrong
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.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:1309
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
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
Daniel van Vugt (vanvugt) wrote : | # |
(4) It's still dangerous to assume mir_pixel_
102 + return (format > mir_pixel_
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.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
SUCCESS: http://
Alexandros Frantzis (afrantzis) wrote : | # |
163 +#include <gmock/gmock.h>
Still not needed, but not critical.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
SUCCESS: http://
PS Jenkins bot (ps-jenkins) : | # |
Preview Diff
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 | 1 | /* | ||
6 | 2 | * Copyright © 2014 Canonical Ltd. | ||
7 | 3 | * | ||
8 | 4 | * This program is free software: you can redistribute it and/or modify it | ||
9 | 5 | * under the terms of the GNU Lesser General Public License version 3, | ||
10 | 6 | * as published by the Free Software Foundation. | ||
11 | 7 | * | ||
12 | 8 | * This program is distributed in the hope that it will be useful, | ||
13 | 9 | * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
14 | 10 | * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
15 | 11 | * GNU Lesser General Public License for more details. | ||
16 | 12 | * | ||
17 | 13 | * You should have received a copy of the GNU Lesser General Public License | ||
18 | 14 | * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
19 | 15 | * | ||
20 | 16 | * Authored by: Andreas Pokorny <andreas.pokorny@canonical.com> | ||
21 | 17 | */ | ||
22 | 18 | |||
23 | 19 | #ifndef MIR_GRAPHICS_PIXEL_FORMAT_UTILS_H_ | ||
24 | 20 | #define MIR_GRAPHICS_PIXEL_FORMAT_UTILS_H_ | ||
25 | 21 | |||
26 | 22 | #include "mir_toolkit/common.h" | ||
27 | 23 | |||
28 | 24 | namespace mir | ||
29 | 25 | { | ||
30 | 26 | namespace graphics | ||
31 | 27 | { | ||
32 | 28 | |||
33 | 29 | /*! | ||
34 | 30 | * \name MirPixelFormat utility functions | ||
35 | 31 | * | ||
36 | 32 | * A set of functions to query details of MirPixelFormat | ||
37 | 33 | * TODO improve this through https://bugs.launchpad.net/mir/+bug/1236254 | ||
38 | 34 | * \{ | ||
39 | 35 | */ | ||
40 | 36 | bool contains_alpha(MirPixelFormat format); | ||
41 | 37 | int red_channel_depth(MirPixelFormat format); | ||
42 | 38 | int blue_channel_depth(MirPixelFormat format); | ||
43 | 39 | int green_channel_depth(MirPixelFormat format); | ||
44 | 40 | int alpha_channel_depth(MirPixelFormat format); | ||
45 | 41 | bool valid_pixel_format(MirPixelFormat format); | ||
46 | 42 | /*! | ||
47 | 43 | * \} | ||
48 | 44 | */ | ||
49 | 45 | |||
50 | 46 | |||
51 | 47 | } | ||
52 | 48 | } | ||
53 | 49 | |||
54 | 50 | #endif | ||
55 | 0 | 51 | ||
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 | 10 | display_configuration.cpp | 10 | display_configuration.cpp |
61 | 11 | null_display_report.cpp | 11 | null_display_report.cpp |
62 | 12 | buffer_basic.cpp | 12 | buffer_basic.cpp |
63 | 13 | pixel_format_utils.cpp | ||
64 | 13 | ) | 14 | ) |
65 | 14 | 15 | ||
66 | 15 | add_library( | 16 | add_library( |
67 | 16 | 17 | ||
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 | 1 | /* | ||
73 | 2 | * Copyright © 2014 Canonical Ltd. | ||
74 | 3 | * | ||
75 | 4 | * This program is free software: you can redistribute it and/or modify it | ||
76 | 5 | * under the terms of the GNU Lesser General Public License version 3, | ||
77 | 6 | * as published by the Free Software Foundation. | ||
78 | 7 | * | ||
79 | 8 | * This program is distributed in the hope that it will be useful, | ||
80 | 9 | * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
81 | 10 | * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
82 | 11 | * GNU Lesser General Public License for more details. | ||
83 | 12 | * | ||
84 | 13 | * You should have received a copy of the GNU Lesser General Public License | ||
85 | 14 | * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
86 | 15 | * | ||
87 | 16 | * Authored by: Andreas Pokorny <andreas.pokorny@canonical.com> | ||
88 | 17 | */ | ||
89 | 18 | |||
90 | 19 | #include "mir/graphics/pixel_format_utils.h" | ||
91 | 20 | |||
92 | 21 | namespace mg = mir::graphics; | ||
93 | 22 | |||
94 | 23 | bool mg::contains_alpha(MirPixelFormat format) | ||
95 | 24 | { | ||
96 | 25 | return (format == mir_pixel_format_abgr_8888 || | ||
97 | 26 | format == mir_pixel_format_argb_8888); | ||
98 | 27 | } | ||
99 | 28 | |||
100 | 29 | bool mg::valid_pixel_format(MirPixelFormat format) | ||
101 | 30 | { | ||
102 | 31 | return (format > mir_pixel_format_invalid && | ||
103 | 32 | format < mir_pixel_formats); | ||
104 | 33 | } | ||
105 | 34 | |||
106 | 35 | int mg::red_channel_depth(MirPixelFormat format) | ||
107 | 36 | { | ||
108 | 37 | return valid_pixel_format(format) ? 8 : 0; | ||
109 | 38 | } | ||
110 | 39 | |||
111 | 40 | int mg::blue_channel_depth(MirPixelFormat format) | ||
112 | 41 | { | ||
113 | 42 | return valid_pixel_format(format) ? 8 : 0; | ||
114 | 43 | } | ||
115 | 44 | |||
116 | 45 | int mg::green_channel_depth(MirPixelFormat format) | ||
117 | 46 | { | ||
118 | 47 | return valid_pixel_format(format) ? 8 : 0; | ||
119 | 48 | } | ||
120 | 49 | |||
121 | 50 | int mg::alpha_channel_depth(MirPixelFormat format) | ||
122 | 51 | { | ||
123 | 52 | return contains_alpha(format) ? 8 : 0; | ||
124 | 53 | } | ||
125 | 54 | |||
126 | 0 | 55 | ||
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 | 6 | ${CMAKE_CURRENT_SOURCE_DIR}/test_default_display_configuration_policy.cpp | 6 | ${CMAKE_CURRENT_SOURCE_DIR}/test_default_display_configuration_policy.cpp |
132 | 7 | ${CMAKE_CURRENT_SOURCE_DIR}/test_buffer_id.cpp | 7 | ${CMAKE_CURRENT_SOURCE_DIR}/test_buffer_id.cpp |
133 | 8 | ${CMAKE_CURRENT_SOURCE_DIR}/test_buffer_properties.cpp | 8 | ${CMAKE_CURRENT_SOURCE_DIR}/test_buffer_properties.cpp |
134 | 9 | ${CMAKE_CURRENT_SOURCE_DIR}/test_pixel_format_utils.cpp | ||
135 | 9 | ${CMAKE_CURRENT_SOURCE_DIR}/test_surfaceless_egl_context.cpp | 10 | ${CMAKE_CURRENT_SOURCE_DIR}/test_surfaceless_egl_context.cpp |
136 | 10 | ) | 11 | ) |
137 | 11 | 12 | ||
138 | 12 | 13 | ||
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 | 1 | /* | ||
144 | 2 | * Copyright © 2014 Canonical Ltd. | ||
145 | 3 | * | ||
146 | 4 | * This program is free software: you can redistribute it and/or modify it | ||
147 | 5 | * under the terms of the GNU General Public License version 3, | ||
148 | 6 | * as published by the Free Software Foundation. | ||
149 | 7 | * | ||
150 | 8 | * This program is distributed in the hope that it will be useful, | ||
151 | 9 | * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
152 | 10 | * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
153 | 11 | * GNU General Public License for more details. | ||
154 | 12 | * | ||
155 | 13 | * You should have received a copy of the GNU General Public License | ||
156 | 14 | * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
157 | 15 | * | ||
158 | 16 | * Authored by: Andreas Pokorny <andreas.pokorny@canonical.com> | ||
159 | 17 | */ | ||
160 | 18 | |||
161 | 19 | #include "mir_toolkit/common.h" | ||
162 | 20 | #include "mir/graphics/pixel_format_utils.h" | ||
163 | 21 | #include <gmock/gmock.h> | ||
164 | 22 | #include <gtest/gtest.h> | ||
165 | 23 | |||
166 | 24 | using namespace mir::graphics; | ||
167 | 25 | TEST(MirPixelFormatUtils, contains_alpha) | ||
168 | 26 | { | ||
169 | 27 | EXPECT_FALSE(contains_alpha(mir_pixel_format_xbgr_8888)); | ||
170 | 28 | EXPECT_FALSE(contains_alpha(mir_pixel_format_bgr_888)); | ||
171 | 29 | EXPECT_FALSE(contains_alpha(mir_pixel_format_xrgb_8888)); | ||
172 | 30 | EXPECT_FALSE(contains_alpha(mir_pixel_format_xbgr_8888)); | ||
173 | 31 | EXPECT_TRUE(contains_alpha(mir_pixel_format_argb_8888)); | ||
174 | 32 | EXPECT_TRUE(contains_alpha(mir_pixel_format_abgr_8888)); | ||
175 | 33 | EXPECT_FALSE(contains_alpha(mir_pixel_format_invalid)); | ||
176 | 34 | EXPECT_FALSE(contains_alpha(mir_pixel_formats)); | ||
177 | 35 | } | ||
178 | 36 | |||
179 | 37 | TEST(MirPixelFormatUtils, red_channel_depths) | ||
180 | 38 | { | ||
181 | 39 | EXPECT_EQ(8, red_channel_depth(mir_pixel_format_xbgr_8888)); | ||
182 | 40 | EXPECT_EQ(8, red_channel_depth(mir_pixel_format_bgr_888)); | ||
183 | 41 | EXPECT_EQ(8, red_channel_depth(mir_pixel_format_xrgb_8888)); | ||
184 | 42 | EXPECT_EQ(8, red_channel_depth(mir_pixel_format_xbgr_8888)); | ||
185 | 43 | EXPECT_EQ(8, red_channel_depth(mir_pixel_format_argb_8888)); | ||
186 | 44 | EXPECT_EQ(8, red_channel_depth(mir_pixel_format_abgr_8888)); | ||
187 | 45 | EXPECT_EQ(0, red_channel_depth(mir_pixel_format_invalid)); | ||
188 | 46 | EXPECT_EQ(0, red_channel_depth(mir_pixel_formats)); | ||
189 | 47 | } | ||
190 | 48 | |||
191 | 49 | TEST(MirPixelFormatUtils, blue_channel_depths) | ||
192 | 50 | { | ||
193 | 51 | EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_xbgr_8888)); | ||
194 | 52 | EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_bgr_888)); | ||
195 | 53 | EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_xrgb_8888)); | ||
196 | 54 | EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_xbgr_8888)); | ||
197 | 55 | EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_argb_8888)); | ||
198 | 56 | EXPECT_EQ(8, blue_channel_depth(mir_pixel_format_abgr_8888)); | ||
199 | 57 | EXPECT_EQ(0, blue_channel_depth(mir_pixel_format_invalid)); | ||
200 | 58 | EXPECT_EQ(0, blue_channel_depth(mir_pixel_formats)); | ||
201 | 59 | } | ||
202 | 60 | |||
203 | 61 | TEST(MirPixelFormatUtils, green_channel_depths) | ||
204 | 62 | { | ||
205 | 63 | EXPECT_EQ(8, green_channel_depth(mir_pixel_format_xbgr_8888)); | ||
206 | 64 | EXPECT_EQ(8, green_channel_depth(mir_pixel_format_bgr_888)); | ||
207 | 65 | EXPECT_EQ(8, green_channel_depth(mir_pixel_format_xrgb_8888)); | ||
208 | 66 | EXPECT_EQ(8, green_channel_depth(mir_pixel_format_xbgr_8888)); | ||
209 | 67 | EXPECT_EQ(8, green_channel_depth(mir_pixel_format_argb_8888)); | ||
210 | 68 | EXPECT_EQ(8, green_channel_depth(mir_pixel_format_abgr_8888)); | ||
211 | 69 | EXPECT_EQ(0, green_channel_depth(mir_pixel_format_invalid)); | ||
212 | 70 | EXPECT_EQ(0, green_channel_depth(mir_pixel_formats)); | ||
213 | 71 | } | ||
214 | 72 | |||
215 | 73 | |||
216 | 74 | TEST(MirPixelFormatUtils, alpha_channel_depths) | ||
217 | 75 | { | ||
218 | 76 | EXPECT_EQ(0, alpha_channel_depth(mir_pixel_format_xbgr_8888)); | ||
219 | 77 | EXPECT_EQ(0, alpha_channel_depth(mir_pixel_format_bgr_888)); | ||
220 | 78 | EXPECT_EQ(0, alpha_channel_depth(mir_pixel_format_xrgb_8888)); | ||
221 | 79 | EXPECT_EQ(0, alpha_channel_depth(mir_pixel_format_xbgr_8888)); | ||
222 | 80 | EXPECT_EQ(8, alpha_channel_depth(mir_pixel_format_argb_8888)); | ||
223 | 81 | EXPECT_EQ(8, alpha_channel_depth(mir_pixel_format_abgr_8888)); | ||
224 | 82 | EXPECT_EQ(0, alpha_channel_depth(mir_pixel_format_invalid)); | ||
225 | 83 | EXPECT_EQ(0, alpha_channel_depth(mir_pixel_formats)); | ||
226 | 84 | } | ||
227 | 85 | |||
228 | 86 | TEST(MirPixelFormatUtils, valid_mir_pixel_format) | ||
229 | 87 | { | ||
230 | 88 | EXPECT_TRUE(valid_pixel_format(mir_pixel_format_xbgr_8888)); | ||
231 | 89 | EXPECT_TRUE(valid_pixel_format(mir_pixel_format_bgr_888)); | ||
232 | 90 | EXPECT_TRUE(valid_pixel_format(mir_pixel_format_xrgb_8888)); | ||
233 | 91 | EXPECT_TRUE(valid_pixel_format(mir_pixel_format_xbgr_8888)); | ||
234 | 92 | EXPECT_TRUE(valid_pixel_format(mir_pixel_format_argb_8888)); | ||
235 | 93 | EXPECT_TRUE(valid_pixel_format(mir_pixel_format_abgr_8888)); | ||
236 | 94 | EXPECT_FALSE(valid_pixel_format(mir_pixel_format_invalid)); | ||
237 | 95 | EXPECT_FALSE(valid_pixel_format(mir_pixel_formats)); | ||
238 | 96 | } |
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: format_ invalid < format &&
100 + if (mir_pixel_
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: MirPixelFormat format) pixel_format( int format)
bool valid_format(
to be:
bool valid_mir_
?
(4) TEST(MirPixelfo rmat, ...
It is more correct and still safe to use the correct capitalization "MirPixelFormat". Although calling the test "PixelFormatUtils" might be even more accurate.