-
Notifications
You must be signed in to change notification settings - Fork 50
OIIO #143
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
base: master
Are you sure you want to change the base?
OIIO #143
Conversation
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
…ibrary Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
@@ -566,7 +529,7 @@ bool ImageConverter::configure( | |||
OIIO::TypeDesc( OIIO::TypeDesc::FLOAT, 4 ), | |||
user_mul ); | |||
|
|||
if ( _read_raw ) | |||
// if ( _read_raw ) |
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.
U sure not left accidentally?
(sorry, I don't know what's happening here at all, but this change of logic flow from outside looks like potential unintentional change)
Or if itentional, then deleting it is a better option?
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.
It is intentional, yes. A temporary fix until I can find a better solution. The problem is we are a bit limited in what we can do when dealing with libraw via OIIO. We don't have the same level of control as in the older version where we could access libraw directly.
|
||
# setup our library dependencies (i.e. ilmbase) | ||
include( "${RAWTOACES_CMAKE_DIR}/RAWTOACESLibraryDepends.cmake" ) | ||
# include( "${RAWTOACES_CMAKE_DIR}/RAWTOACESLibraryDepends.cmake" ) |
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.
another commented out line not to forget about (unless, it's a special syntax in c++, which I don't know. sry if so)
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.
There is no such file in the repo, whoever wrote it has forgotten to push the file, I assume.
// here. This creates a bit of overhead, we may want to decouple | ||
// the selection of illuminant and WB calculation in the core | ||
// library. But this is still faster than running the matrix solver | ||
// everytime. |
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.
Reminder to deal with this TODO before merging. Don't know how you guys do it. At my place we either deal with them right there or create and issue and link to it. If it's ok to have TODO left for future generation - ok then.
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.
The PR is still marked as draft :)
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.
Thanks for taking a look by the way. I will be pushing more changes soon, there are still things missing/broken.
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.
@antond-weta sorry. I will back off =) I would be annoyed. Let me know if you need beta tester (I am a mac user) for changes.
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.
No worries! Just saying that this PR is not going to be merged in any time soon, there are still many things to do.
The code in this branch is mostly functional. There are some bits still missing, like white balancing by averaging all pixels or a box. In any other mode you should get the result identical to the master.
Feel free to use/test, any feedback/suggestions would be appreciated.
b36f004
to
a87817a
Compare
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
Signed-off-by: Anton Dukhovnikov <[email protected]>
This is a proposed replacement of the 'rawtoaces_util' library and 'rawtoaces' command line tool.
Main changes:
The makefile contains 2 new targets rawtoaces_util2 and rawtoaces2. The old targets are still in place.