-
Notifications
You must be signed in to change notification settings - Fork 57
v2 - remove the dependency on libraw from the core library #162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
v2 - remove the dependency on libraw from the core library #162
Conversation
src/rawtoaces_idt/rta.cpp
Outdated
| mir - ccttoMired( XYZToColorTemperature( mulVector( | ||
| invertV( XYZtoCameraWeightedMatrix( mir, mir1, mir2 ) ), | ||
| _neutralRGBDNG ) ) ); | ||
| _metadata.neutralRGB ) ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be coming from argument? so neutralRGB
|
|
||
| #if LIBRAW_VERSION >= LIBRAW_MAKE_VERSION( 0, 20, 0 ) | ||
| metadata.baselineExposure = | ||
| static_cast<double>( color.dng_levels.baseline_exposure ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be unsigned short ?
| std::filesystem::path pathToRaw = std::filesystem::absolute( | ||
| "../../unittest/materials/blackmagic_cinema_camera_cinemadng.dng" ); | ||
| int ret = rawProcessor.open_file( ( pathToRaw.string() ).c_str() ); | ||
| ret = rawProcessor.unpack(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to put BOOST_REQUIRE_EQUAL( ret, LIBRAW_SUCCESS ); after each line where ret = is assigned? (so it would be open_file and upack. For early clear failure.
| #include <unordered_map> | ||
| #include <libraw/libraw.h> | ||
|
|
||
| using namespace rta; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion is to remove this namespace and replace those instances of core::Idt with rta::core::Idt here and in .cpp file
to avoid polluting consumers’ namespaces from a public header. Public headers should not inject namespaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but this is unrelated to the change. Trying to keep the scope of each PR limited.
I generally dislike the using namespace thing, we should get rid of those in a separate PR, at least in the public headers.
| static AcesRender &getPrivateInstance(); | ||
|
|
||
| const AcesRender &operator=( const AcesRender &acesrender ); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion is to move impl. bit of LibRawAces into .cpp
so lines 17-24 replace with class LibRawAces; and move all this into .cpp instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a need for #include <libraw/libraw.h> in here as well? to
avoids those ODR/incomplete-type issues and matches how the types are actually defined by LibRaw.
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
743547a to
f00d813
Compare
64d85ea
into
AcademySoftwareFoundation:master
This removes the libraw data structure being passed to the core library and replaces it with a dedicated structure rta::core::Metadata.
This changes the public interface, so is not backward compatible with v1.x, has to be a major update.