Skip to content
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.

Sign up for GitHub

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

feat(raw): raw reader - exposing max_raw_memory_mb #4454

Merged
merged 4 commits into from
Sep 30, 2024
Merged

feat(raw): raw reader - exposing max_raw_memory_mb #4454

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
159cc36
exposing max_raw_memory_mb
ankmachine Sep 27, 2024
92bcfaf
setting default max to 2G and add documnetation for same
ankmachine Sep 29, 2024
01bba5f
improved documentation
ankmachine Sep 30, 2024
fc55703
images made more sense in doc
ankmachine Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/raw.imageio/rawinput.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,14 @@ RawInput::open_raw(bool unpack, const std::string& name,
// Output 16 bit images
m_processor->imgdata.params.output_bps = 16;

// exposing max_raw_memory_mb setting
static_assert(sizeof(m_processor->imgdata.rawparams.max_raw_memory_mb)
== sizeof(int),
"Sizes must match");
config.getattribute("raw:max_raw_memory_mb", TypeDesc::INT,
&m_processor->imgdata.rawparams.max_raw_memory_mb);


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might it be simpler to express this more like

m_processor->imgdata.rawparams.max_raw_memory_mb = 
    config.get_int_attribute("raw:max_raw_memory_mb", <default value>);

?? Then you wouldn't need the static assert, and various type conversions would happen automatically if the types ever change.

Also, would you mind please documenting the existence and meaning of this attribute in the raw section of builtinplugins.rst ? Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's a good default for this? The other reason I like my suggestion above is that it's probably a good idea to by default impose some reasonable limit, even if this parameter is not passed from the user. We shouldn't allow big files to consume unlimited memory in libraw unless specifically asked to do so.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default in libraw is 2048 (2GB)
https://github.com/LibRaw/LibRaw/blob/9bcb8a1d9593ba67e4eb67fed716efc5e1353d5c/libraw/libraw_const.h#L24
and
https://github.com/LibRaw/LibRaw/blob/9bcb8a1d9593ba67e4eb67fed716efc5e1353d5c/doc/API-datastruct.html#L556-L558
Libraw itself doesn't have a upper limit.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, let's just express that default as I suggested above, then you don't need the static_assert or the needing pointers.

// Disable exposure correction (unless config "raw:auto_bright" == 1)
m_processor->imgdata.params.no_auto_bright
= !config.get_int_attribute("raw:auto_bright", 0);
Expand Down
Loading