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.

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

Metadata #1448

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
129 changes: 106 additions & 23 deletions src/io/metagram.rs → src/io/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ use std::sync::{Arc, Mutex};

use crate::color::Color;

// oder (Engram), Metagram,
/// Collects some arbitrary meta data of an image.
/// Collects some arbitrary metadata of an image.
///
/// Note that information collected here will, per default, appear opaque to the `image` library
/// itself. For example, the width and height of an image might be recorded as one set of values
Expand All @@ -13,7 +12,7 @@ use crate::color::Color;
/// recheck with the specific decoder if you require color accuracy beyond presuming sRGB. (This
/// may be improved upon in the future, if you have a concrete draft please open an issue).
#[derive(Clone, Default)]
pub struct Metagram {
pub struct MetadataContainer {
/// The original width in pixels.
pub width: u32,
/// The original height in pixels.
Expand All @@ -26,10 +25,37 @@ pub struct Metagram {
_non_exhaustive: (),
}

/// A buffer for the extra meta data produced by an image decoder.
/// Hints whether one specific metadatum is requested.
#[derive(Clone, Copy)]
pub enum DatumRequested {
/// The decoder should skip the datum when encountered.
Ignore,
/// The decoder should decode, validate, and add the datum.
Record,
}

/// Configure the caller needs of metadata.
///
/// Not every piece of metadata can be added for free. For example, reading EXIF profiles is
/// commonly associated with decoding of additional chunks, validation, and additional resource
/// usage. All of these can be opt-out by allowing such a customization. Note that, nevertheless,
/// the absence of metadata is not definitive proof of its absence in the original file. It might
/// be more recent than the decoder and not yet have support.
#[derive(Clone, Default)]
// We may want to add a non-copy field?
#[allow(missing_copy_implementations)]
pub struct RecorderConfig {
/// Whether the decoder should try to provide color profile data.
pub color_profile: DatumRequested,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason this couldn't be pub skip_color_profile: bool?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it took me a while to make up my own mind. My vague idea was to have a third variant, Required, that would fail decoding when no information is available. This could allow decoders to fail fast instead of needing to wait for the final check which would complement Limits nicely. However, there are some pain points:

  • We will have to add a check after the decoder in any case if we want to make the an actionable guarantee for the caller. But when those checks trigger we will merely discard already correct data which is rather odd and mostly detrimental.
  • If we have any sort of recovery or fallback then we need to relax it to Requested in any case with similar concerns as above.
  • There might be overlapping constraints such as 'ensure a correct, complete and accurate record of all color conversion and quantization steps in the image creation pipeline' which wouldn't fit in the tri-state idea in any case.
  • They can always be converted to a flag on the recorded MetadataContainer. Consider the above then we could add a bool attribute that signals the completeness of the color information in other fields.

This leaves only code clarity as a potential benefit of an explicit new enum.

/// Whether the decoder should try to provide EXIF data.
pub exif: DatumRequested,
_non_exhaustive: (),
}

/// A buffer for the extra metadata produced by an image decoder.
///
/// There are two main ways of creation: Any consumer of the `ImageDecoder` interface can create
/// their own recorder to retrieve meta data from the decoder. A decoder wrapping another format
/// their own recorder to retrieve metadata from the decoder. A decoder wrapping another format
/// (i.e. jpeg-within-tiff) might create a recorder from a `SharedRecorder` to add additional data
/// to the outer recorder instead.
///
Expand All @@ -44,7 +70,7 @@ pub struct Metagram {
/// # fn color_type(&self) -> image::ColorType { todo!() }
/// # fn into_reader(self) -> image::ImageResult<std::io::Empty> { Ok(std::io::empty()) }
/// # }
/// use image::io::{Metagram, Recorder};
/// use image::io::{MetadataContainer, Recorder};
///
/// let mut some_decoder = // ..
/// # FakeDecoder;
Expand All @@ -58,33 +84,50 @@ pub struct Metagram {
/// some_decoder.read_image(&mut _buffer);
/// }
///
/// let meta: Metagram = recorder.to_result();
/// let meta: MetadataContainer = recorder.to_result();
/// ```
pub struct Recorder {
inner: RecorderInner,
config: RecorderConfig,
}

enum RecorderInner {
Owned(Box<RefCell<Metagram>>),
Shared(Arc<Mutex<Metagram>>),
Owned(Box<RefCell<MetadataContainer>>),
Shared(Arc<Mutex<MetadataContainer>>),
}

/// An owned handle to a `Metagram`, that allows concurrent modification.
/// An owned handle to a `MetadataContainer`, that allows concurrent modification.
#[allow(missing_copy_implementations)]
pub struct SharedRecorder {
inner: Arc<Mutex<Metagram>>,
inner: Arc<Mutex<MetadataContainer>>,
config: RecorderConfig,
}

impl Recorder {
/// Create a recorder recording into a new, empty meta data.
/// Create a recorder recording into a new, empty metadata.
pub fn new() -> Self {
Recorder::default()
}

/// Get the current configuration.
pub fn config(&self) -> &RecorderConfig {
&self.config
}

/// Get a mutable reference to the current configuration.
///
/// Note that the configuration is cloned when converting between `Recorder` and
/// `SharedRecorder` but it is not _shared_ between instances. That is, changes only apply to
/// this instance and not to clones.
pub fn config_mut(&mut self) -> &mut RecorderConfig {
&mut self.config
}

/// Create a record that already contains some data.
pub fn with(meta: Metagram) -> Self {
pub fn with(meta: MetadataContainer) -> Self {
Recorder {
inner: RecorderInner::Owned(Box::new(RefCell::new(meta))),
config: RecorderConfig::default(),
}
}

Expand Down Expand Up @@ -113,36 +156,38 @@ impl Recorder {

SharedRecorder {
inner,
config: self.config.clone(),
}
}

/// Get a clone of the configured meta data.
pub fn to_result(&self) -> Metagram {
/// Get a clone of the configured metadata.
pub fn to_result(&self) -> MetadataContainer {
self.inner.to_result()
}
}

impl RecorderInner {
fn with_mut(&self, function: impl FnOnce(&mut Metagram)) {
fn with_mut(&self, function: impl FnOnce(&mut MetadataContainer)) {
match self {
RecorderInner::Owned(boxed) => {
function(&mut boxed.borrow_mut())
}
RecorderInner::Shared(arc) => {
function(&mut arc.lock().unwrap())
function(&mut arc.lock().unwrap_or_else(|err| err.into_inner()))
}
}
}

fn to_result(&self) -> Metagram {
fn to_result(&self) -> MetadataContainer {
match self {
RecorderInner::Owned(boxed) => boxed.borrow().clone(),
RecorderInner::Shared(arc) => arc.lock().unwrap().clone(),
}
}
}

/// Setters for recording meta data.
/// Setters for recording metadata.
///
/// Any change here should also be made for `SharedRecorder` unless it is specifically not possible
/// to perform on a shared, and locked struct.
impl Recorder {
Expand All @@ -160,9 +205,32 @@ impl Recorder {
pub fn exif(&self, data: Vec<u8>) {
self.inner.with_mut(|meta| meta.set_exif(data))
}

/// Mutate the metadata container.
///
/// This will make the changes visible to all other recorders sharing the container target. It
/// is not specified what happens when `function` panics but there will be no undefined
/// behavior.
pub fn with_mut(&self, function: impl FnOnce(&mut MetadataContainer)) {
self.inner.with_mut(function)
}
}

impl SharedRecorder {
/// Get the current configuration.
pub fn config(&self) -> &RecorderConfig {
&self.config
}

/// Get a mutable reference to the current configuration.
///
/// Note that the configuration is cloned when converting between `Recorder` and
/// `SharedRecorder` but it is not _shared_ between instances. That is, changes only apply to
/// this instance and not to clones.
pub fn config_mut(&mut self) -> &mut RecorderConfig {
&mut self.config
}

/// Add original dimensions.
pub fn dimensions(&self, width: u32, height: u32) {
self.with_mut(|meta| meta.set_dimensions((width, height)));
Expand All @@ -178,7 +246,12 @@ impl SharedRecorder {
self.with_mut(|meta| meta.set_exif(data))
}

fn with_mut(&self, function: impl FnOnce(&mut Metagram)) {
/// Mutate the shared metadata container.
///
/// This will make the changes visible to all other recorders sharing the container target. It
/// is not specified what happens when `function` panics but there will be no undefined
/// behavior.
pub fn with_mut(&self, function: impl FnOnce(&mut MetadataContainer)) {
// Regarding lock recovery: None of the inner methods should usually panic. The only
// exception would be from allocation error while inserting a new exif tag or something.
function(&mut self.inner.lock().unwrap_or_else(|err| err.into_inner()))
Expand All @@ -187,7 +260,7 @@ impl SharedRecorder {

/// Private implementation of metagram, existing for the purpose of make assignment available as
/// methods.
impl Metagram {
impl MetadataContainer {
fn set_dimensions(&mut self, (width, height): (u32, u32)) {
self.width = width;
self.height = height;
Expand All @@ -206,15 +279,25 @@ impl Default for Recorder {
fn default() -> Self {
Recorder {
inner: RecorderInner::Owned(Default::default()),
config: RecorderConfig::default(),
}
}
}

/// Convert a shared recorder into a non-thread safe variant.
/// The two will _still_ record to the same meta data collection but this new instances could be
/// The two will _still_ record to the same metadata collection but this new instances could be
/// used as a method argument to another `ImageDecoder` impl.
impl From<SharedRecorder> for Recorder {
fn from(shared: SharedRecorder) -> Recorder {
Recorder { inner: RecorderInner::Shared(shared.inner) }
Recorder {
inner: RecorderInner::Shared(shared.inner),
config: shared.config,
}
}
}

impl Default for DatumRequested {
fn default() -> Self {
DatumRequested::Record
}
}
4 changes: 2 additions & 2 deletions src/io/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Input and output of images.
mod metagram;
mod metadata;
mod reader;
pub(crate) mod free_functions;

pub use self::reader::Reader;
pub use self::metagram::{Metagram, Recorder, SharedRecorder};
pub use self::metadata::{DatumRequested, MetadataContainer, Recorder, RecorderConfig, SharedRecorder};