Reduce tag_filter to a minimum, implement copy

src/encoder/mod.rs

@@ -2,27 +2,27 @@ pub use tiff_value::*;
 
 use std::{
     cmp,
-    io::{self, Seek, Write},
+    io::{self, Read, Seek, Write},
     marker::PhantomData,
     mem,
     num::{NonZeroU64, TryFromIntError},
 };
 
 use crate::{
-    decoder::{ifd::Entry, IfdDecoder},
+    decoder::{ifd::Entry, Decoder},
     error::{TiffResult, UsageError},
     tags::{
         ByteOrder, CompressionMethod, ExtraSamples, IfdPointer, PhotometricInterpretation,
-        ResolutionUnit, SampleFormat, Tag, Type, ValueBuffer,
+        PlanarConfiguration, ResolutionUnit, SampleFormat, Tag, Type, ValueBuffer,
     },
     Directory, TiffError, TiffFormatError,
 };
 
 pub mod colortype;
 pub mod compression;
+mod tag_filter;
 mod tiff_value;
 mod writer;
-mod tag_filter;
 
 use self::colortype::*;
 use self::compression::Compression as Comp;
@@ -438,16 +438,142 @@ impl<'a, W: 'a + Write + Seek, K: TiffKind> DirectoryEncoder<'a, W, K> {
     }
 
     /// Copy metadata from the IFD that a decoder refers to.
-    pub fn write_metadata_from(
+    ///
+    /// You generally want to use [`ImageEncoder::write_metadata_copy_from`] which automatically
+    /// fills out the [`EncodeMetadataApplicability`] as applicable to the chosen color space and
+    /// other sample configuration.
+    pub fn write_metadata_copy_from<R: Read + Seek>(
         &mut self,
-        mut decoder: IfdDecoder<'_>,
+        decoder: &mut Decoder<R>,
         options: EncodeMetadataOptions<'_>,
+        kind: EncodeMetadataApplicability,
     ) -> TiffResult<EncodeMetadataReport> {
         let mut report = EncodeMetadataReport {
+            copied: 0,
+            filtered: 0,
         };
 
-        for tag in decoder.tag_iter() {
+        enum Role {
-            let (tag, entry) = tag?;
+            Ifd,
+            ExifPrivate,
+            GpsPrivate,
+        }
+
+        let ptr = decoder.ifd_pointer().unwrap();
+        let mut stack = vec![(ptr, Role::Ifd, usize::MAX)];
+        let mut buf = ValueBuffer::empty(Type::UNDEFINED);
+
+        // Directories which we will later write in reverse-pre-order.
+        let mut pre_order_dirs: Vec<(Directory, usize, Tag)> = vec![];
+
+        while let Some((ifd, role, parent)) = stack.pop() {
+            let dir = decoder.read_directory(ifd)?;
+            let mut created_dir = Directory::empty();
+
+            let filter = match role {
+                Role::Ifd => TagFilter::filter_primary,
+                Role::ExifPrivate => TagFilter::filter_exif_private,
+                Role::GpsPrivate => TagFilter::filter_gps_private,
+            };
+
+            let mut ifd_reader = decoder.read_directory_tags(&dir);
+
+            for (tag, entry) in dir.iter() {
+                let level = filter(options.tag_filter, tag.to_u16());
+                match options
+                    .tag_filter
+                    .should_keep_for_image_writer(level, &kind)
+                {
+                    tag_filter::Choice::Ok => {}
+                    tag_filter::Choice::Descend(subifd_kind) => {
+                        // Only single pointers can be directories to descend to, avoid an overly
+                        // large read by pulling that value out of the file.
+                        if entry.count() != 1 {
+                            report.filtered = report.filtered.saturating_add(1);
+                            continue;
+                        }
+
+                        if ifd_reader.find_tag_buf(tag, &mut buf)?.is_none() {
+                            // Should not happen (we're iterating the dict already), but be robust.
+                            report.filtered = report.filtered.saturating_add(1);
+                            continue;
+                        };
+
+                        let Some(ifd) = buf.as_ifd_pointer() else {
+                            report.filtered = report.filtered.saturating_add(1);
+                            continue;
+                        };
+
+                        let role = match subifd_kind {
+                            tag_filter::SubifdKind::ExifPrivate => Role::ExifPrivate,
+                            tag_filter::SubifdKind::Gps => Role::GpsPrivate,
+                            tag_filter::SubifdKind::Interoperability => {
+                                // Only applicable to compressed images.
+                                report.filtered = report.filtered.saturating_add(1);
+                                continue;
+                            }
+                        };
+
+                        let this_as_parent = pre_order_dirs.len();
+                        stack.push((ifd, role, this_as_parent));
+                    }
+                    tag_filter::Choice::Discard => continue,
+                    tag_filter::Choice::Filtered
+                    // Not supported yet.
+                    | tag_filter::Choice::Special(tag_filter::Special::JpegThumbnail) => {
+                        // Not to be copied.
+                        report.filtered = report.filtered.saturating_add(1);
+                        continue;
+                    }
+                    tag_filter::Choice::Special(tag_filter::Special::CompressedExclusive) => {
+                        // We're not writing an App segment for a compressed file, are we?
+                        continue;
+                    }
+                }
+
+                if ifd_reader.find_tag_buf(tag, &mut buf)?.is_none() {
+                    report.filtered = report.filtered.saturating_add(1);
+                    continue;
+                }
+
+                // Re-encode the value to the target file.
+                buf.set_byte_order(ByteOrder::native());
+                let entry = self.write_entry_buf(&buf)?;
+                created_dir.extend([(tag, entry)]);
+                report.copied = report.copied.saturating_add(1);
+            }
+
+            let tag_in_parent = match role {
+                // should be unreachable? For now. But thumbnails would use this so we best be a
+                // little cautious here. Also thumbnails are weird, they do have image data but we
+                // would not copy that data itself with these methods. Just eh.
+                //
+                // Anyways the tree root will use this tag as a dummy.
+                Role::Ifd => Tag::SubIfd,
+                Role::ExifPrivate => Tag::ExifDirectory,
+                Role::GpsPrivate => Tag::GpsDirectory,
+            };
+
+            pre_order_dirs.push((created_dir, parent, tag_in_parent));
+        }
+
+        // Now post-order traverse the directory tree.
+        while let Some((directory, parent, tag)) = pre_order_dirs.pop() {
+            // The root is the items for the IFD itself.
+            if pre_order_dirs.is_empty() {
+                // We do not enter ourselves into a parent directory.
+                let _ = (parent, tag);
+
+                self.extend_from(&directory);
+                break;
+            }
+
+            let offset = Self::write_directory_inner(self.writer, &directory)?;
+            let offset = K::convert_offset(offset)?;
+            let entry = self.write_entry(offset)?;
+
+            let (parent_dir, _, _) = &mut pre_order_dirs[parent];
+            parent_dir.extend([(tag, entry)]);
         }
 
         Ok(report)
@@ -479,17 +605,21 @@ impl<'a, W: 'a + Write + Seek, K: TiffKind> DirectoryEncoder<'a, W, K> {
     }
 
     fn write_directory(&mut self) -> TiffResult<u64> {
+        Self::write_directory_inner(self.writer, &self.directory)
+    }
+
+    fn write_directory_inner(writer: &mut TiffWriter<W>, directory: &Directory) -> TiffResult<u64> {
         // Start by turning all buffered unwritten values into entries.
-        let offset = self.writer.offset();
+        let offset = writer.offset();
-        K::write_entry_count(self.writer, self.directory.len())?;
+        K::write_entry_count(writer, directory.len())?;
 
         let offset_bytes = mem::size_of::<K::OffsetType>();
-        for (tag, entry) in self.directory.iter() {
+        for (tag, entry) in directory.iter() {
-            self.writer.write_u16(tag.to_u16())?;
+            writer.write_u16(tag.to_u16())?;
-            self.writer.write_u16(entry.field_type().to_u16())?;
+            writer.write_u16(entry.field_type().to_u16())?;
             let count = K::convert_offset(entry.count())?;
-            count.write(self.writer)?;
+            count.write(writer)?;
-            self.writer.write_bytes(&entry.offset()[..offset_bytes])?;
+            writer.write_bytes(&entry.offset()[..offset_bytes])?;
         }
 
         Ok(offset)
@@ -617,16 +747,46 @@ pub struct EncodeMetadataOptions<'lt> {
     /// Setting this flag will skip such unknown tags instead of attempting a potentially incorrect
     /// copy. However the set of copied metadata is then dependent on the `tiff` library.
     pub tag_filter: &'lt TagFilter,
-    /// Some tags are not allowed to be recorded or can be semantically validated. The library can
+    // TODO:
-    /// skip these tags while copying metadata. For instance, `YCbCrCoefficients` _may_ be
+    // Some tags are not allowed to be recorded or can be semantically validated. The library can
-    /// recorded for images in `YCbCr` photometric interpretation but _must not_ be recorded for
+    // skip these tags while copying metadata. For instance, `YCbCrCoefficients` _may_ be
-    /// images in any other interpretation (colorspace conversion should have been done during the
+    // recorded for images in `YCbCr` photometric interpretation but _must not_ be recorded for
-    /// transformation).
+    // images in any other interpretation (colorspace conversion should have been done during the
-    pub skip_invalid_tags: bool,
+    // transformation).
+    // pub validate_tags: enum ValidateTags {
+    //   None,
+    //   Skip,
+    //   Error,
+    //   Strict /* Error but also do not allow anything that can not be validated */
+    // },
+}
+
+impl Default for EncodeMetadataOptions<'_> {
+    fn default() -> Self {
+        EncodeMetadataOptions {
+            tag_filter: TagFilter::only_known_tags(),
+        }
+    }
+}
+
+/// Describe the kind of image for which metadata is recorded.
+///
+/// Metadata that is not applicable to the color space or the layout may be skipped. For instance,
+/// subsampling basis position is only relevant on YCbCr / Chroma color photometric interpretation
+/// where subsampling is defined.
+#[non_exhaustive]
+#[derive(Clone, Copy, Default)]
+pub struct EncodeMetadataApplicability {
+    pub photometric: Option<PhotometricInterpretation>,
+    pub planar: Option<PlanarConfiguration>,
 }
 
 #[non_exhaustive]
 pub struct EncodeMetadataReport {
+    /// Number of tags transferred (recursively).
+    pub copied: u64,
+    /// Number of tags encountered that were skipped due to the provided filter.
+    pub filtered: u64,
 }
 
 /// Type to encode images strip by strip.
@@ -869,6 +1029,22 @@ impl<'a, W: 'a + Write + Seek, T: ColorType, K: TiffKind> ImageEncoder<'a, W, T,
         Ok(())
     }
 
+    /// Copy metadata from the IFD that a decoder refers to.
+    pub fn write_metadata_copy_from<R: Read + Seek>(
+        &mut self,
+        decoder: &mut Decoder<R>,
+        options: EncodeMetadataOptions<'_>,
+    ) -> TiffResult<EncodeMetadataReport> {
+        self.encoder.write_metadata_copy_from(
+            decoder,
+            options,
+            EncodeMetadataApplicability {
+                photometric: Some(<T>::TIFF_VALUE),
+                planar: Some(PlanarConfiguration::Chunky),
+            },
+        )
+    }
+
     /// Set image resolution
     pub fn resolution(&mut self, unit: ResolutionUnit, value: Rational) {
         self.encoder

src/encoder/tag_filter.rs

@@ -24,18 +24,6 @@ impl TagFilter {
         &INSTANCE
     }
 
-    /// Return a filter that attempts to remove identifying tags. This is stricter than
-    /// [`Self::only_known_tags`] and excludes many tags except those describing the image data
-    /// layout.
-    ///
-    /// **Note that it comes with *no warranty*.** although this will remove tags with prejudice.
-    pub fn attempt_at_privacy() -> &'static TagFilter {
-        static INSTANCE: TagFilter = TagFilter {
-            filter_level: FilterLevel::Privacy,
-        };
-        &INSTANCE
-    }
-
     /// FIXME: take a configuration by which to filter out complete metadata kinds.
     /// FIXME: when we iterate over a whole directory of tags, this iteration is more expensive
     /// than necessary. Tags within a directory should be ordered. We can thus perform a binary set
@@ -168,32 +156,15 @@ impl TagFilter {
         }
     }
 
-    fn filter_interoperability(&self, tag: u16) -> Level {
+    pub(crate) fn should_keep_for_image_writer(
-        match tag {
-            0x0001 => Level::Special(Special::CompressedExclusive),
-            _ => Level::Unknown,
-        }
-    }
-
-    pub(crate) fn filter_secondary_exif(&self, tag: u16) -> Level {
-        // No difference for now. The only tabular difference I could spot is the treatment of
-        // sample positioning which is mandatory in the primary IFD of YCbCr but optional for all
-        // following IFDs..
-        self.filter_primary_exif(tag)
-    }
-
-    pub(crate) fn filter_secondary_gps(&self, tag: u16) -> Level {
-        self.filter_primary_gps(tag)
-    }
-
-    pub(crate) fn should_keep_on_level(
         &self,
         level: Level,
         target: &super::EncodeMetadataApplicability,
     ) -> Choice {
         match level {
-            Level::Unknown if matches!(self.filter_level, FilterLevel::All) => Choice::Ok,
+            Level::Unknown if matches!(self.filter_level, FilterLevel::All) => Choice::Filtered,
-            Level::SampleLayout => Choice::Ok,
+            // Sample Layout is controlled by the writer (dimensions, color type, …). Do not copy!
+            Level::SampleLayout => Choice::Discard,
             Level::Ifd(subifd_kind) => Choice::Descend(subifd_kind),
             Level::Photometric(photometric) => {
                 match (photometric, target.planar, target.photometric) {
@@ -213,15 +184,7 @@ impl TagFilter {
                     _ => Choice::Discard,
                 }
             }
-            Level::Value if matches!(self.filter_level, FilterLevel::Privacy) => Choice::Discard,
             Level::Value => Choice::Ok,
-            Level::Special(Special::JpegThumbnail)
-                if matches!(self.filter_level, FilterLevel::Privacy) =>
-            {
-                // Contains arbitrary unverified other data, so, no, we do not heed backward
-                // compatibility of a tag that should no longer be in active use if possible.
-                Choice::Discard
-            }
             Level::Special(special) => Choice::Special(special),
             _ => Choice::Discard,
         }
@@ -231,7 +194,10 @@ impl TagFilter {
 pub(crate) enum Choice {
     Ok,
     Descend(SubifdKind),
+    /// Tag is inapplicable to be copied.
     Discard,
+    /// Tag was chosen not to be copied.
+    Filtered,
     Special(Special),
 }
 
@@ -239,7 +205,6 @@ pub(crate) enum Choice {
 enum FilterLevel {
     All,
     Known,
-    Privacy,
 }
 
 /// The level of support for a value.