-
Notifications
You must be signed in to change notification settings - Fork 99
feat: impl ReaderBuilder
#1465
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: main
Are you sure you want to change the base?
feat: impl ReaderBuilder
#1465
Conversation
…nch 'origin' into ok-nick/settings-builders
|
Still needs docs and tests but marking as ready for further discussion. |
…ttings-not-thread-local
| } | ||
| } | ||
|
|
||
| pub fn external_manifest( |
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.
This should be set_external_manifest(), otherwise it looks like a getter in Rust.
| &self, | ||
| init_stream: &mut dyn CAIRead, | ||
| fragment_paths: &Vec<std::path::PathBuf>, | ||
| fragment_paths: &mut [Box<dyn CAIRead>], |
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.
We are generally moving away from CaiRead and CaiWrite as we don't need them anymore. But since this function already uses CAIRead, I think this is ok here.
| pub struct ReaderBuilder { | ||
| settings: Settings, | ||
| external_manifest: Option<Box<dyn CAIRead>>, |
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.
I think this could be done as a ReaderAsset and include format and stream.
then we can call something like this on Reader
pub fn from_reader_asset(
settings: &Settings,
asset: ReaderAsset,
) -> Result<Reader> {
let format = asset.format;
let stream = asset.stream;
if let Some(external_manifest) = asset.external_manifest {
if let Some(fragments) = asset.fragments {
Self::from_fragment(
&format,
stream,
external_manifest,
fragments,
)
} else {
Self::from_manifest_data_and_stream(
&jumbf_io::read_to_end(external_manifest)?,
&format,
stream,
)
}
} else {
Self::from_stream(&format, stream)
}
}
|
Are there currently plans to expose this object to the c2pa_c_ffi path? I would love to be able to access this from binding code. |
Implements a
ReaderBuilderfor constructing aReaderwith settings and various other options. We may want to consider introducing a similar pattern to theBuilder, although that may take additional design consideration.For some background, we need methods of passing in
Settingsto aReaderorBuilderthat are instance-local, rather than thread-local. There are numerous reasons why this is needed that have taken place in other discussions. The reason we decided to introduce aReaderBuilderis because it's impractical and arguably unidiomatic to create or change ~9 of theReaderconstructors. It also offers much more flexibility and enables many new permutations of constructing aReader.Note that this is only an investigation, we may consider going the other route of providing an
Assetstruct to encapsulate assets, allowing us to cache a map of the files internals so we don't need to parse it multiple times.Blocked by: