Enum class for image types #737

piponazo · 2019-03-04T18:25:04Z

After the first positive impressions gathered in #732, I have created this PR in which I just focus in the addition of the enum class ImageType to replace the previous namespace in which we were adding global constant values.

Note that the characterisation tests have revealed some bugs that I also fixed in this branch.

If you are happy with the changes, and somebody approves the PR, feel free to merge the branch. I will not be available for some days.

D4N

Don't get me wrong, I really like this change, but currently this actually modifies how the library behaves (probably only in subtle ways and you are actually improving matters here, so keep reading).

I went spelunking in the code searching for the big switch(ImageType) {} and wondering how that would work since some image types shared the numeric code. Well, I found it, it's at the top of src/image.cpp: const Registry registry[]. Please take a look at the code, but in a nutshell that is an array of structs that contain a "unique ID" (=ImageType), a function pointer to a constructor and some flags.

The problem is: currently the code chooses for the appropriate constructor by using std::find on registry and comparing the image types. That's of completely wrong, since the different image type "codes" that are the same have different flags! For instance a srw image has currently ImageType = 4 and the flags indicate read-write support. But sr2 images have also id 4 but only supports reading... So one of the two will get the wrong flags.

So: Thank you for switching to enum class, otherwise we would have never found that. Now, what do we do about this? We have two choices:

Keep the numeric ids the same and retain the same (probably) buggy behavior.
or
Use the PR in it's current form and thereby actually fix ImageFactory.

I vote for 2, but that requires at least some additional tests verifying that this works.

And the MinGW build is now broken :(

D4N · 2019-03-04T20:59:10Z

unitTests/test_ImageFactory.cpp

+
+TEST(TheImageFactory, createsInstancesForSupportedTypesInMemory)
+{
+    // Note that the constructor of these Image classes take an 'create' argument


What does this comment mean?

I understand that this comment is quite cryptic ...

I discovered with these characterisation tests that ImageFactory::create throws for those Image Types which are not using a create boolean parameter in their constructor ... Compare these two constructors and their related tests:

// Constructors JpegImage(BasicIo::UniquePtr io, bool create); explicit BmpImage(BasicIo::UniquePtr io); // Tests // Note that the constructor of these Image classes take an 'create' argument EXPECT_NO_THROW(ImageFactory::create(ImageType::jpeg)); // Note that the constructor of these Image classes does not take an 'create' argument EXPECT_THROW(ImageFactory::create(ImageType::bmp), Error);

My guess is that some of the authors took care of implementing the functionality for being able to create and save metadata for some Image Types, while others did not do it for some formats (and therefore only the loading functionality is implemented; and writing from existing metadata).

Oh boy, now I understand.

D4N · 2019-03-04T21:12:45Z

include/exiv2/image_types.hpp

+
+namespace Exiv2 {
+    /// Supported Image Formats
+    enum class ImageType{


EDIT: See the review comment for another view on this.

This is a pretty big API change, as now any client side code that relies on ImageType == 5 being equivalent to ImageType == ImageType::mrw will stop working.

Don't take me wrong, this is definitely the right way forward! enum class is for nearly all purposes superior to the classical enum types. However, I think we should keep the numbers the same.

I know this is a big API change but as you already have mentioned the previous behaviour was buggy.

The main problem I see is that it is not possible to keep the same numbers. Take a look here:
https://github.com/Exiv2/exiv2/pull/737/files#diff-5c74f56289aad5a5f43de672abf604a8L40

We had several types sharing the same value ... 🤦‍♂️

What I could do is to leave the tiff type with value 4, then continue with the other values that was not define in that file and put at the end of the Enum Type the types:

dng, nef, pef, arw, sr2, srw,

What do you think?

If we break the API, break it good so that people who have hardcoded the numbers will find out ASAP.

After continuing working on this, I do not think users will suffer a lot about this change. In case they were using the previous ImageType namespace, they will need to change it for using the new scoped enum in the form of ImageType::jpeg. But that's all ... the ImageType type is not used in the API except for the ImageFactory class and not in the most common functions.

I think we can keep that Enum class ordered alphabetically, and the numbers associated to each Image Type will not be very relevant.

unitTests/test_ImageFactory.cpp

include/exiv2/image_types.hpp

piponazo · 2019-03-13T16:07:16Z

So: Thank you for switching to enum class, otherwise we would have never found that. Now, what do we do about this? We have two choices: 1) Keep the numeric ids the same and retain the same (probably) buggy behavior. or 2) Use the PR in it's current form and thereby actually fix ImageFactory.
I vote for 2, but that requires at least some additional tests verifying that this works.

I also noticed the Registry stuff and the buggy behaviour, but I forgot to mention it in the PR description. I agree with you in the 2nd choice. I will invest a bit more of time to improve the situation by:

Create new characterisation tests revealing the issue we have found.
Fix the situation.
Adapt the characterisation tests to unit tests describing the new behaviour.

clanmills · 2019-03-13T17:10:48Z

I think we should fix the registry/enum. Option 2.

Going for #pragma once in C++11? Yes. Go for it. Remove the historical/ugly #ifdef BLA_BLA_H code.

D4N · 2019-03-13T22:10:51Z

So: Thank you for switching to enum class, otherwise we would have never found that. Now, what do we do about this? We have two choices: 1) Keep the numeric ids the same and retain the same (probably) buggy behavior. or 2) Use the PR in it's current form and thereby actually fix ImageFactory.
I vote for 2, but that requires at least some additional tests verifying that this works.

I also noticed the Registry stuff and the buggy behaviour, but I forget to mention it in the PR description. I agree with you in the 2nd choice. I will invest a bit more of time to improve the situation by:

Create new characterisation tests revealing the issue we have found.

Fix the situation.

Adapt the characterisation tests to unit tests describing the new behaviour.

Sounds great!

codecov · 2019-03-14T18:52:59Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ea4a4fe). Click here to learn what that means.
The diff coverage is 99.29%.

@@            Coverage Diff            @@
##             master     #737   +/-   ##
=========================================
  Coverage          ?   70.45%           
=========================================
  Files             ?      144           
  Lines             ?    19193           
  Branches          ?        0           
=========================================
  Hits              ?    13523           
  Misses            ?     5670           
  Partials          ?        0

Impacted Files	Coverage Δ
include/exiv2/jpgimage.hpp	`100% <ø> (ø)`
include/exiv2/pngimage.hpp	`100% <ø> (ø)`
include/exiv2/psdimage.hpp	`100% <ø> (ø)`
include/exiv2/xmpsidecar.hpp	`100% <ø> (ø)`
include/exiv2/tgaimage.hpp	`100% <ø> (ø)`
include/exiv2/webpimage.hpp	`100% <ø> (ø)`
include/exiv2/jp2image.hpp	`100% <ø> (ø)`
include/exiv2/image.hpp	`100% <ø> (ø)`
src/actions.cpp	`73.38% <ø> (ø)`
include/exiv2/cr2image.hpp	`100% <ø> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea4a4fe...a3d6fe0. Read the comment docs.

piponazo · 2019-03-14T19:03:02Z

I added the tests I promised and I actually did not need to do anything else. Just the single fact of changing to a scoped enum fixed the situation, since it guarantees to have unique integer values associated. All the tests I added worked as I expected.

Previously we had some image types sharing the same integer value as @D4N pointed out, and I am sure that this was a buggy behaviour associated.

I do not think this bug should worry us in the 0.27 branch. I guess that most of the users are using ImageFactory::load with a string containing the image path. The bug is affecting those methods of ImageFactory taking ImageType as parameter.

stale

D4N

Overall great work!

Could you just clarify why you started using the EXPECT_* macros instead of assert? Also, MinGW is still broken :(

unitTests/test_ImageFactory.cpp

D4N · 2019-03-14T19:30:33Z

src/image.cpp

@@ -95,6 +95,7 @@ namespace {
        AccessMode     commentSupport_;
    };

+    /// \todo Use std::unordered_map for implementing the registry. Avoid to use ImageType::none


std::array is imho better here, each element has a unique id and these ids are sequential starting at zero. There's not really a better datastructure for this than an array.

Only if you know size at compile time.

We do, the index is ImageType.

It depends. If we want to conserve a relationship between integer values and ImageTypes, then yes. But is this something we should do? I do not think we need to do it ...

As I commented previously, I do not think this change will have an impact on users since those ImageType values are used mostly internally. I do not see how a user would want to rely in those values in external software. Therefore, I think that std::unordered_map would be a better option with ImageType as key, an a Register instance as value. The order of the ImageType would not matter for the Registry implementation in that way, and we could keep that scope enum sorted alphabetically.

Anyways, I just left the comment there to address this later, and we could discuss about the best way to implement that in other issue/PR 😉

piponazo · 2019-03-15T22:15:07Z

Grrr the situation with the cross-compilation is not fixed. I will need to investigate a bit to be able to compile with mingw locally and document the process. Currently I am having some problems to do it locally.

D4N · 2019-03-16T12:02:03Z

Grrr the situation with the cross-compilation is not fixed. I will need to investigate a bit to be able to compile with mingw locally and document the process. Currently I am having some problems to do it locally.

If you install the mingw packages (see .gitlab-ci.yml) inside Fedora vagrant box, then you should be able to reproduce the issues locally. To me it looks like mingw lacks a specialization of operator << with output streams for ImageType.

- Split test identifying which Images can be created and which not - Add tests for the ImageFactory::create(type, filePath)

…pes)

piponazo · 2019-03-17T09:44:52Z

@D4N Finally the issue with the MinGW compilation is solved. I just needed to cast the enum class type to integer for that output operator.

clanmills · 2019-03-17T11:31:30Z

@piponazo @D4N You have both shown great determination and tenancy on this. Outstanding. Thank You for making the effort.

You'll have seen that @cryptomilk has joined us on Slack. He's going to tutor me on git. I will release v0.27.1 RC1 this week.

I'm working with @nehaljwani on the servers which are now on-line. I'm going to ask Andreas Huggel to let me pay to keep exiv2.org alive on AWS. I can use the new servers in in England for Jenkins and release candidates. AWS is stable and reliable, so I'd like to keep that alive.

piponazo added the enhancement feature / functionality enhancements label Mar 4, 2019

piponazo self-assigned this Mar 4, 2019

piponazo requested review from clanmills, D4N and tbeu March 4, 2019 18:25

D4N previously requested changes Mar 4, 2019

View reviewed changes

piponazo force-pushed the EnumClassForImageTypes branch from b2388a5 to 9456922 Compare March 14, 2019 18:52

piponazo requested a review from D4N March 14, 2019 18:53

D4N reviewed Mar 14, 2019

View reviewed changes

piponazo force-pushed the EnumClassForImageTypes branch from 77954a9 to d74ea12 Compare March 17, 2019 07:14

piponazo added 12 commits March 17, 2019 08:16

Add characterisation test for ImageFactory::create(type)

0a21a5b

- Split test identifying which Images can be created and which not - Add tests for the ImageFactory::create(type, filePath)

No need to delete move operations

033818e

Replace ImageType namespace by a enum class

edd6fb4

No need anymore to include each image header to use the ImageFactory

5f66948

Prevent call to std::memcpy with src == nullptr

4ebea5b

New tests to check what happens with ImageFactory::create(crazyValue)

1a53707

bug: Fix crash when calling ImageFactory::create(none)

b838726

test ImageFactory::load with existing images in test/data

c94a3ad

Add tests for ImageFactory::checkMode (for all the supported image ty…

abb3c26

…pes)

Remove useless comments at the end of functions

1a4c2af

Change order of ImageType values

f521975

Make include guard of image_types.h 'more unique'

f2f9a89

piponazo added 2 commits March 17, 2019 08:16

Should fix mingw compilation

aa47a57

Replace EXPECT_XXX to ASSERT___

a3d6fe0

piponazo force-pushed the EnumClassForImageTypes branch from d74ea12 to a3d6fe0 Compare March 17, 2019 07:16

piponazo requested a review from D4N March 17, 2019 09:44

tbeu approved these changes Mar 17, 2019

View reviewed changes

piponazo merged commit a3c996e into master Mar 17, 2019

piponazo deleted the EnumClassForImageTypes branch March 17, 2019 16:33

piponazo mentioned this pull request Feb 13, 2022

New ImageTypes enum class #2093

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enum class for image types #737

Enum class for image types #737

piponazo commented Mar 4, 2019

D4N left a comment

D4N Mar 4, 2019

piponazo Mar 13, 2019

D4N Mar 13, 2019

D4N Mar 4, 2019 •

edited

Loading

piponazo Mar 13, 2019

D4N Mar 13, 2019

piponazo Mar 14, 2019

piponazo commented Mar 13, 2019 •

edited

Loading

clanmills commented Mar 13, 2019

D4N commented Mar 13, 2019

codecov bot commented Mar 14, 2019 •

edited

Loading

piponazo commented Mar 14, 2019

D4N left a comment

D4N Mar 14, 2019

tbeu Mar 14, 2019

D4N Mar 15, 2019

piponazo Mar 15, 2019

piponazo commented Mar 15, 2019

D4N commented Mar 16, 2019

piponazo commented Mar 17, 2019

clanmills commented Mar 17, 2019

Enum class for image types #737

Enum class for image types #737

Conversation

piponazo commented Mar 4, 2019

D4N left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D4N Mar 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piponazo commented Mar 13, 2019 • edited Loading

clanmills commented Mar 13, 2019

D4N commented Mar 13, 2019

codecov bot commented Mar 14, 2019 • edited Loading

Codecov Report

piponazo commented Mar 14, 2019

D4N left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piponazo commented Mar 15, 2019

D4N commented Mar 16, 2019

piponazo commented Mar 17, 2019

clanmills commented Mar 17, 2019

D4N Mar 4, 2019 •

edited

Loading

piponazo commented Mar 13, 2019 •

edited

Loading

codecov bot commented Mar 14, 2019 •

edited

Loading