Preserve display aspect ratio for videos with rectangular samples

4 files changed, 62 insertions, 19 deletions

diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md
index 5e01297fc1..061898d143 100644
--- a/activestorage/CHANGELOG.md
+++ b/activestorage/CHANGELOG.md
@@ -1,3 +1,12 @@
+*   Preserve display aspect ratio when extracting width and height from videos
+    with rectangular samples in `ActiveStorage::Analyzer::VideoAnalyzer`.
+
+    When a video contains a display aspect ratio, emit it in metadata as
+    `:display_aspect_ratio` rather than the ambiguous `:aspect_ratio`. Compute
+    its height by scaling its encoded frame width according to the DAR.
+
+    *George Claghorn*
+
 *   Use `after_destroy_commit` instead of `before_destroy` for purging
     attachments when a record is destroyed.
 
diff --git a/activestorage/lib/active_storage/analyzer/video_analyzer.rb b/activestorage/lib/active_storage/analyzer/video_analyzer.rb
index aa532da201..f0d9baa199 100644
--- a/activestorage/lib/active_storage/analyzer/video_analyzer.rb
+++ b/activestorage/lib/active_storage/analyzer/video_analyzer.rb
@@ -9,12 +9,12 @@ module ActiveStorage
   # * Height (pixels)
   # * Duration (seconds)
   # * Angle (degrees)
-  # * Aspect ratio
+  # * Display aspect ratio
   #
   # Example:
   #
   #   ActiveStorage::VideoAnalyzer.new(blob).metadata
-  #   # => { width: 640, height: 480, duration: 5.0, angle: 0, aspect_ratio: [4, 3] }
+  #   # => { width: 640.0, height: 480.0, duration: 5.0, angle: 0, display_aspect_ratio: [4, 3] }
   #
   # When a video's angle is 90 or 270 degrees, its width and height are automatically swapped for convenience.
   #
@@ -25,24 +25,24 @@ module ActiveStorage
     end
 
     def metadata
-      { width: width, height: height, duration: duration, angle: angle, aspect_ratio: aspect_ratio }.compact
+      { width: width, height: height, duration: duration, angle: angle, display_aspect_ratio: display_aspect_ratio }.compact
     end
 
     private
       def width
-        rotated? ? raw_height : raw_width
+        if rotated?
+          computed_height || encoded_height
+        else
+          encoded_width
+        end
       end
 
       def height
-        rotated? ? raw_width : raw_height
-      end
-
-      def raw_width
-        Integer(video_stream["width"]) if video_stream["width"]
-      end
-
-      def raw_height
-        Integer(video_stream["height"]) if video_stream["height"]
+        if rotated?
+          encoded_width
+        else
+          computed_height || encoded_height
+        end
       end
 
       def duration
@@ -53,16 +53,36 @@ module ActiveStorage
         Integer(tags["rotate"]) if tags["rotate"]
       end
 
-      def aspect_ratio
+      def display_aspect_ratio
         if descriptor = video_stream["display_aspect_ratio"]
-          descriptor.split(":", 2).collect(&:to_i)
+          terms = descriptor.split(":", 2).collect(&:to_i)
+          terms if terms.count == 2 && terms.min >= 0
         end
       end
 
+
       def rotated?
         angle == 90 || angle == 270
       end
 
+      def computed_height
+        if encoded_width && display_height_scale
+          encoded_width * display_height_scale
+        end
+      end
+
+      def encoded_width
+        @encoded_width ||= Float(video_stream["width"]) if video_stream["width"]
+      end
+
+      def encoded_height
+        @encoded_height ||= Float(video_stream["height"]) if video_stream["height"]
+      end
+
+      def display_height_scale
+        @display_height_scale ||= Float(display_aspect_ratio.last) / display_aspect_ratio.first if display_aspect_ratio
+      end
+
 
       def tags
         @tags ||= video_stream["tags"] || {}
diff --git a/activestorage/test/analyzer/video_analyzer_test.rb b/activestorage/test/analyzer/video_analyzer_test.rb
index 7fc1cfce06..03fbd72735 100644
--- a/activestorage/test/analyzer/video_analyzer_test.rb
+++ b/activestorage/test/analyzer/video_analyzer_test.rb
@@ -8,28 +8,42 @@ require "active_storage/analyzer/video_analyzer"
 class ActiveStorage::Analyzer::VideoAnalyzerTest < ActiveSupport::TestCase
   test "analyzing a video" do
     blob = create_file_blob(filename: "video.mp4", content_type: "video/mp4")
-    metadata = blob.tap(&:analyze).metadata
+    metadata = extract_metadata_from(blob)
 
     assert_equal 640, metadata[:width]
     assert_equal 480, metadata[:height]
-    assert_equal [4, 3], metadata[:aspect_ratio]
+    assert_equal [4, 3], metadata[:display_aspect_ratio]
     assert_equal 5.166648, metadata[:duration]
     assert_not_includes metadata, :angle
   end
 
   test "analyzing a rotated video" do
     blob = create_file_blob(filename: "rotated_video.mp4", content_type: "video/mp4")
-    metadata = blob.tap(&:analyze).metadata
+    metadata = extract_metadata_from(blob)
 
     assert_equal 480, metadata[:width]
     assert_equal 640, metadata[:height]
-    assert_equal [4, 3], metadata[:aspect_ratio]
+    assert_equal [4, 3], metadata[:display_aspect_ratio]
     assert_equal 5.227975, metadata[:duration]
     assert_equal 90, metadata[:angle]
   end
 
+  test "analyzing a video with rectangular samples" do
+    blob = create_file_blob(filename: "video_with_rectangular_samples.mp4", content_type: "video/mp4")
+    metadata = extract_metadata_from(blob)
+
+    assert_equal 1280, metadata[:width]
+    assert_equal 720, metadata[:height]
+    assert_equal [16, 9], metadata[:display_aspect_ratio]
+  end
+
   test "analyzing a video without a video stream" do
     blob = create_file_blob(filename: "video_without_video_stream.mp4", content_type: "video/mp4")
     assert_equal({ "analyzed" => true, "identified" => true }, blob.tap(&:analyze).metadata)
   end
+
+  private
+    def extract_metadata_from(blob)
+      blob.tap(&:analyze).metadata
+    end
 end
diff --git a/activestorage/test/fixtures/files/video_with_rectangular_samples.mp4 b/activestorage/test/fixtures/files/video_with_rectangular_samples.mp4
new file mode 100644
index 0000000000..12b04afc87
--- /dev/null
+++ b/activestorage/test/fixtures/files/video_with_rectangular_samples.mp4