Add support for Qfactor#308
Conversation
|
Thank you @palemieux for putting this PR. |
| } | ||
|
|
||
| if (qfactor != -1 && quantization_step != -1.0f) | ||
| OJPH_ERROR(0x01000101, |
There was a problem hiding this comment.
Please change all 0x0100010x to 0x010000Ax.
| @@ -1,5 +1,5 @@ | |||
|
|
|||
| file(GLOB CODESTREAM "codestream/*.cpp" "codestream/*.h") | |||
| file(GLOB CODESTREAM "codestream/*.cpp" "codestream/*.h" "codestream/*.hpp") | |||
There was a problem hiding this comment.
I would be nicer if we can change the name to .h so it matches the rest of the code; what do you think?
| class OJPH_EXPORT param_qcd | ||
| { | ||
| public: | ||
| enum comp_type { |
There was a problem hiding this comment.
It would nice to associate a type to the enum, as in
enum comp_type : ui8 {}; or enum comp_type : ui32 {};
If we knew for sure that visual weighting or qfactor will not be assigned to any component other than the first three, we would not need this.
| bool is_qcc_needed(ui32 comp_num, const param_cod &cod, const param_siz &siz); | ||
| void set_delta(float delta) { base_delta = delta; } | ||
| void set_delta(ui32 comp_idx, float delta); | ||
| void set_qfactor(ui32 comp_idx, ui8 qfactor, ojph::param_qcd::comp_type ctype); |
There was a problem hiding this comment.
Is it necessary to pass ojph::param_qcd::comp_type ctype. We can use the comp_idx for this purpose.
It unlikely, although possible, that we will use visual weighting for components other than the first 3.
Perhaps, changing the order is useful to
void set_qfactor(ui32 comp_idx, ojph::param_qcd::comp_type ctype, ui8 qfactor);
What do you think?
There was a problem hiding this comment.
I expect Qfactor may be applied to channels beyond the first three:
- At least in the case of OpenEXR, where images routinely have dozens of channels, with potentially more than 3 containing visual information
- Qfactor can be extended to cover non YCbCr channels by removing/changing the visual weights
Will change the order of the parameters.
| float delta_ref = 0; | ||
| float G_c = 1; | ||
| const open_htj2k::visual_weighting_params vp; | ||
| std::vector<double> W_b; |
There was a problem hiding this comment.
This std::vector objects call new. I would really like to remove them, even if that meant I have to modify Osamu's library. Ultimately, these are tables with a known number of entries.
The main ojph library have very limited calls to new.
I know Osamu attempts to limit the new calls to one per vector by using reserve.
The horrors of C++.
There was a problem hiding this comment.
Good point.
Questions:
- would you be happy with
reserve()to avoid multiple calls to new()? - are you happy with
std::array?
There was a problem hiding this comment.
I think std::array is a good idea.
For the time being, I will merge the PR, but then I will modify Osamu's code. Do you have any objections?
There was a problem hiding this comment.
For the time being, I will merge the PR, but then I will modify Osamu's code. Do you have any objections?
Zero objection assuming that you are ok with submitting a PR against Osamu's library with your proposed improvements.
2 participants
To improve interoperability, the feature uses the visual weights library from @osamu620 at https://github.com/osamu620/OpenHTJ2K/blob/main/source/core/codestream/visual_weighting.hpp, which is copied into OpenJPH.