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

Makes it optional to load JPEG thumbnails from Tiff files #70

Merged
merged 4 commits into from May 26, 2023
Merged

Makes it optional to load JPEG thumbnails from Tiff files #70

merged 4 commits into from May 26, 2023

Conversation

rcpalacio
Copy link
Contributor

Hi

Some use cases of EXIFR don't actually require loading JPEG thumbnails from TIFF files.

As files are getting larger and larger recently, so are the thumbnails. This means we can save quite a lot of I/O operations in scenarios where these thumbnails are not necessary and throughput is critical.

@remvee
Copy link
Owner

remvee commented May 24, 2023

Hi @rcpalacio, Thank you for your contribution! I'd prefer a keyword argument for this and I'm open to breaking compatibility with ruby-1.9 for this. So what about adding:

diff --git a/lib/exifr/tiff.rb b/lib/exifr/tiff.rb
index a50765a..133efdd 100644
--- a/lib/exifr/tiff.rb
+++ b/lib/exifr/tiff.rb
@@ -375,7 +375,7 @@ module EXIFR
     TAGS = [TAG_MAPPING.keys, TAG_MAPPING.values.map{|v|v.values}].flatten.uniq - IFD_TAGS
 
     # +file+ is a filename or an +IO+ object.  Hint: use +StringIO+ when working with slurped data like blobs.
-    def initialize(file, load_thumbnails = true)
+    def initialize(file, load_thumbnails: true)
       Data.open(file) do |data|
         @ifds = [IFD.new(data)]
         while ifd = @ifds.last.next
diff --git a/tests/tiff_test.rb b/tests/tiff_test.rb
index 76be654..ddf845a 100755
--- a/tests/tiff_test.rb
+++ b/tests/tiff_test.rb
@@ -190,7 +190,7 @@ class TIFFTest < TestCase
 
   def test_skippable_jpeg_thumbnails
     all_test_tiffs.each do |fname|
-      t = TIFF.new(fname, false)
+      t = TIFF.new(fname, load_thumbnails: false)
       assert t.jpeg_thumbnails.empty?
     end
   end

Also, let's not forget about JPEG by adding:

diff --git a/lib/exifr/jpeg.rb b/lib/exifr/jpeg.rb
index aa7d3e9..19d16ec 100644
--- a/lib/exifr/jpeg.rb
+++ b/lib/exifr/jpeg.rb
@@ -29,11 +29,11 @@ module EXIFR
     attr_reader :app1s
 
     # +file+ is a filename or an IO object.  Hint: use StringIO when working with slurped data like blobs.
-    def initialize(file)
+    def initialize(file, load_thumbnails: true)
       if file.kind_of? String
-        File.open(file, 'rb') { |io| examine(io) }
+        File.open(file, 'rb') { |io| examine(io, load_thumbnails: true) }
       else
-        examine(file.dup)
+        examine(file.dup, load_thumbnails: true)
       end
     end
 
@@ -121,7 +121,7 @@ module EXIFR
 
       if app1 = @app1s.find { |d| d[0..5] == "Exif\0\0" }
         @exif_data = app1[6..-1]
-        @exif = TIFF.new(StringIO.new(@exif_data))
+        @exif = TIFF.new(StringIO.new(@exif_data), load_thumbnails: load_thumbnails)
       end
     end
   end

WDYT?

@rcpalacio
Copy link
Contributor Author

Thanks for looking into it so quickly!
I definitely agree, it reads better. I'll incorporate those the changes.

@rcpalacio
Copy link
Contributor Author

I wonder if the failure on ruby 2.2 is related to this issue: ruby/setup-ruby#496

About 1.9, I'll drop it from .github/workflows/test.yml.

@remvee remvee merged commit 87f1943 into remvee:master May 26, 2023
12 of 13 checks passed
@remvee
Copy link
Owner

remvee commented May 26, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants