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

Add support for iterating over all tags #255

Merged
merged 4 commits into from
Nov 26, 2024
Merged

Add support for iterating over all tags #255

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
840f9b6
Add support for iterating over all tags
gschulze Nov 20, 2024
395a755
Use 'sort_by_key' for sorting tags in tests
gschulze Nov 24, 2024
0670fbb
Simplify implementation of 'tag_iter()'
gschulze Nov 24, 2024
fd2bd31
Document 'tag_iter()' method
gschulze Nov 25, 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
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ categories = ["multimedia::images", "multimedia::encoding"]
exclude = ["tests/images/*", "tests/fuzz_images/*"]

[dependencies]
either = "1.13.0"
weezl = "0.1.0"
jpeg = { package = "jpeg-decoder", version = "0.3.0", default-features = false }
flate2 = "1.0.20"
Expand Down
25 changes: 20 additions & 5 deletions src/decoder/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,25 @@
use std::collections::{HashMap, HashSet};
use std::io::{self, Read, Seek};

use crate::{
bytecast, ColorType, TiffError, TiffFormatError, TiffResult, TiffUnsupportedError, UsageError,
};
use either::Either;

use self::ifd::Directory;
use self::image::Image;
use crate::decoder::tag_iter::TagIter;
use crate::tags::{
CompressionMethod, PhotometricInterpretation, PlanarConfiguration, Predictor, SampleFormat,
Tag, Type,
};
use crate::{
bytecast, ColorType, TiffError, TiffFormatError, TiffResult, TiffUnsupportedError, UsageError,
};

use self::ifd::Directory;
use self::image::Image;
use self::stream::{ByteOrder, EndianReader, SmartReader};

pub mod ifd;
mod image;
mod stream;
mod tag_iter;
mod tag_reader;

/// Result of a decoding process
Expand Down Expand Up @@ -895,6 +898,18 @@ impl<R: Read + Seek> Decoder<R> {
self.get_tag(tag)?.into_string()
}

pub fn tag_iter(&mut self) -> impl Iterator<Item = TiffResult<(Tag, ifd::Value)>> + '_ {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also add a short doc comment explaining what this method does? Something like "Returns an iterator over all tags in the current image"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

match self.image().ifd.as_ref() {
None => Either::Left(std::iter::empty()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know when this case happens? Wondering if we should return None or an error rather than an empty iterator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea, honestly. I think from a developer perspective, it is more convenient to have an empty iterator instead of having to deal with an Option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I went through the existing code. It should be impossible for ifd to be None here, so please just .unwrap() it

Some(ifd) => Either::Right(TagIter::new(
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than constructing a TagIter here, I think we could directly return:

ifd.clone().into_iter().map(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried but failed to make the borrow checker happy with this approach. Happy to change it if you can provide a working solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to get it to work with:

        self.image.ifd.as_ref().unwrap().iter().map(|(tag, entry)| {
            entry
                .val(&self.limits, self.bigtiff, &mut self.reader)
                .map(|value| (*tag, value))
        })

(You have to directly access the self.image field rather than calling the self.image() method to make the borrow check happy. There was a reason why the two are separate, though I don't remember what it is right now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, cool, thx! I incorporated your changes and pushed the branch.

ifd.clone(),
&self.limits,
self.bigtiff,
&mut self.reader,
)),
}
}

fn check_chunk_type(&self, expected: ChunkType) -> TiffResult<()> {
if expected != self.image().chunk_type {
return Err(TiffError::UsageError(UsageError::InvalidChunkType(
Expand Down
52 changes: 52 additions & 0 deletions src/decoder/tag_iter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
use std::collections::hash_map::IntoIter;
use std::io::{Read, Seek};

use crate::decoder::ifd::{Directory, Value};
use crate::decoder::stream::SmartReader;
use crate::decoder::{ifd, Limits};
use crate::tags::Tag;
use crate::TiffResult;

pub(crate) struct TagIter<'a, R>
where
R: Read + Seek,
{
iter: IntoIter<Tag, ifd::Entry>,
limits: &'a Limits,
bigtiff: bool,
reader: &'a mut SmartReader<R>,
}

impl<'a, R> TagIter<'a, R>
where
R: Read + Seek,
{
pub fn new(
directory: Directory,
limits: &'a Limits,
bigtiff: bool,
reader: &'a mut SmartReader<R>,
) -> Self {
Self {
iter: directory.into_iter(),
limits,
bigtiff,
reader,
}
}
}

impl<'a, R> Iterator for TagIter<'a, R>
where
R: Read + Seek,
{
type Item = TiffResult<(Tag, Value)>;

fn next(&mut self) -> Option<Self::Item> {
self.iter.next().map(|(tag, entry)| {
entry
.val(self.limits, self.bigtiff, self.reader)
.map(|value| (tag, value))
})
}
}
82 changes: 82 additions & 0 deletions tests/encode_images.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,47 @@ fn encode_decode() {
let mut decoder = Decoder::new(&mut file).unwrap();
assert_eq!(decoder.colortype().unwrap(), ColorType::RGB(8));
assert_eq!(decoder.dimensions().unwrap(), (100, 100));

let mut all_tags = decoder
.tag_iter()
.filter_map(Result::ok)
.collect::<Vec<_>>();
all_tags.sort_by_key(|(t, _)| t.to_u16());
assert_eq!(
all_tags,
vec![
(Tag::ImageWidth, ifd::Value::Unsigned(100)),
(Tag::ImageLength, ifd::Value::Unsigned(100)),
(
Tag::BitsPerSample,
ifd::Value::List(vec![
ifd::Value::UnsignedBig(8),
ifd::Value::UnsignedBig(8),
ifd::Value::UnsignedBig(8)
])
),
(Tag::Compression, ifd::Value::Unsigned(1)),
(Tag::PhotometricInterpretation, ifd::Value::Unsigned(2)),
(Tag::StripOffsets, ifd::Value::Unsigned(8)),
(Tag::SamplesPerPixel, ifd::Value::Unsigned(3)),
(Tag::RowsPerStrip, ifd::Value::Unsigned(3334)),
(Tag::StripByteCounts, ifd::Value::Unsigned(30000)),
(Tag::XResolution, ifd::Value::Rational(1, 1)),
(Tag::YResolution, ifd::Value::Rational(1, 1)),
(Tag::ResolutionUnit, ifd::Value::Unsigned(1)),
(Tag::Artist, ifd::Value::Ascii("Image-tiff".into())),
(Tag::Predictor, ifd::Value::Unsigned(1)),
(
Tag::SampleFormat,
ifd::Value::List(vec![
ifd::Value::UnsignedBig(1),
ifd::Value::UnsignedBig(1),
ifd::Value::UnsignedBig(1)
])
),
]
);

assert_eq!(
decoder.get_tag(Tag::Artist).unwrap(),
ifd::Value::Ascii("Image-tiff".into())
Expand Down Expand Up @@ -75,6 +116,47 @@ fn encode_decode_big() {
let mut decoder = Decoder::new(&mut file).unwrap();
assert_eq!(decoder.colortype().unwrap(), ColorType::RGB(8));
assert_eq!(decoder.dimensions().unwrap(), (100, 100));

let mut all_tags = decoder
.tag_iter()
.filter_map(Result::ok)
.collect::<Vec<_>>();
all_tags.sort_by_key(|(t, _)| t.to_u16());
assert_eq!(
all_tags,
vec![
(Tag::ImageWidth, ifd::Value::Unsigned(100)),
(Tag::ImageLength, ifd::Value::Unsigned(100)),
(
Tag::BitsPerSample,
ifd::Value::List(vec![
ifd::Value::Short(8),
ifd::Value::Short(8),
ifd::Value::Short(8)
])
),
(Tag::Compression, ifd::Value::Unsigned(1)),
(Tag::PhotometricInterpretation, ifd::Value::Unsigned(2)),
(Tag::StripOffsets, ifd::Value::UnsignedBig(16)),
(Tag::SamplesPerPixel, ifd::Value::Unsigned(3)),
(Tag::RowsPerStrip, ifd::Value::Unsigned(3334)),
(Tag::StripByteCounts, ifd::Value::UnsignedBig(30000)),
(Tag::XResolution, ifd::Value::Rational(1, 1)),
(Tag::YResolution, ifd::Value::Rational(1, 1)),
(Tag::ResolutionUnit, ifd::Value::Unsigned(1)),
(Tag::Artist, ifd::Value::Ascii("Image-tiff".into())),
(Tag::Predictor, ifd::Value::Unsigned(1)),
(
Tag::SampleFormat,
ifd::Value::List(vec![
ifd::Value::Short(1),
ifd::Value::Short(1),
ifd::Value::Short(1)
])
),
]
);

assert_eq!(
decoder.get_tag(Tag::Artist).unwrap(),
ifd::Value::Ascii("Image-tiff".into())
Expand Down