Cleanup image metadata reading

[p12tic]

This PR removes a couple of functions from mvsData that do the same thing as functions in image module. Additionally, readImageSpec function has been introduced and all functions were redefined in terms of it, which makes them simplier (no need to define additional variables just to satisfy an interface like before). Finally, there is now a way to get whatever attributes needed without writing any more getter functions.

The objective is to have a single place for all metadata access.

The new readImageSpec() function is not strictly necessary. However, there was void readImageSpec(const std::string& path, int& width, int& height, int& nchannels); overload before, so given that nchannels was needed at least at some point in the past, it seems logical to expose the underlying OIIO ImageSpec without restrictions to satisfy similar accesses in the future.

[p12tic]

Addressed constness comments. It would have been enough to point just one case and add "and elsewhere", I would have fixed all occurrences.

About constness itself, personally I don't like splattering const to local non-reference non-pointer variables, because it prevents few bugs, yet leads to real issues down the road. For example, const auto x = ...; return x; - const here forbids compiler to use move-constructor for the return value. If x is of expensive type then the code will be unnecessarily slower. In cases of virtual functions, const arguments forces the overriding functions to copy variables in case modification was sensible in that particular overriding function. And overall, too much const causes visual clutter. Const pointer must become const T* const x for consistency and so on.

But I understand why you would like to have const everywhere as a matter of policy, so feel free to disregard my comment :-) I will try to use const more in subsequent PRs.

[fabiencastan]

IMO, const helps readability. Each declaration without a const means that it will be modified, that's very useful to quickly read an algorithm.

I like the simplification that this PR is providing! Just need to see if it's not a problem to start creating dependencies between "mvsData/mvsUtils" and "image". When we fuse the image duplication, we will obviously have this dependency. In the meantime, I'm not sure if it's good to create dependencies between modules that contain duplicates. I would be more confident to do the move in one PR to avoid the indermediate status, that is worse than the current one. Or if we are sure that we have the bandwidth to do the larger change quickly. But we also have the problem of the depthmap branch that will not be ready before 3 weeks.

[p12tic]

IMO, const helps readability. Each declaration without a const means that it will be modified, that's very useful to quickly read an algorithm.

Fair enough.

I'm not sure if it's good to create dependencies between modules that contain duplicates. I would be more confident to do the move in one PR to avoid the indermediate status, that is worse than the current one.

I think I can perform the refactor without introducing worse intermediate status.

In this particular case there are no real duplicates, the functions are all named differently or in different namespaces, except that they do similar things. So looking from C++ code perspective there is no risk.

I have fixed all real duplicate symbols when preparing https://github.com/alicevision/AliceVision/pull/1208.

[p12tic]

I will prepare a single PR with all changes.