fix picture metadata parsing errors (#3123)

* fix picture metadata parsing errors

use exiftool as a fallback when exiv2 fails

SDCP-1061

Author: petrjasek

superdesk/media/image.py

@@ -165,10 +165,9 @@ def get_meta_iptc(file_stream: BinaryIO):
         except KeyError:
             continue
         if isinstance(value, list):
-            value = [decode(v) for v in value]
+            metadata[tag] = [decode(v) for v in value]
         elif isinstance(value, bytes):
-            value = decode(value)
-        metadata[tag] = value
+            metadata[tag] = decode(value)
     return metadata
 
 
@@ -177,8 +176,18 @@ def read_metadata(input: bytes) -> MediaMetadata:
 
     @param file_stream: stream
     """
-    with pyexiv2.ImageData(input) as img:
-        xmp = img.read_xmp()
+    try:
+        with pyexiv2.ImageData(input) as img:
+            xmp = img.read_xmp()
+    except (RuntimeError, ValueError) as e:
+        logger.warning(f"Failed to read image metadata with pyexiv2: {e}, trying exiftool fallback", exc_info=True)
+        # Try to use exiftool as fallback when pyexiv2 fails
+        # Import here to avoid loading exiftool dependencies unless needed (lazy loading)
+        from superdesk.media.video import read_metadata as read_metadata_video
+
+        # Map video module metadata to image module field names
+        return _map_video_to_image(read_metadata_video(input))
+
     return {
         "Description": get_xmp_lang_string(xmp.get("Xmp.dc.description")),
         "DescriptionWriter": xmp.get("Xmp.photoshop.CaptionWriter", ""),
@@ -197,6 +206,57 @@ def read_metadata(input: bytes) -> MediaMetadata:
     }
 
 
+# Field mappings between image module and video module field names
+# Keys are video module names, values are image module names
+_FIELD_MAPPING: Dict[str, str] = {
+    "CaptionWriter": "DescriptionWriter",
+    "TransmissionReference": "JobId",
+    "AuthorsPosition": "CreatorsJobtitle",
+    "Rights": "CopyrightNotice",
+    "State": "ProvinceState",
+    "Credit": "CreditLine",
+}
+
+
+def _map_video_to_image(metadata: MediaMetadata) -> MediaMetadata:
+    """Map video module metadata field names to image module field names.
+
+    Converts video module field names to image module field names for consistency.
+
+    @param metadata: Metadata dict from exiftool fallback
+    @return: Metadata with image module field names
+    """
+    result: MediaMetadata = {}
+
+    for key, value in metadata.items():
+        # Use mapped name if available, otherwise keep original name
+        mapped_key = _FIELD_MAPPING.get(key, key)
+        result[mapped_key] = value  # type: ignore[literal-required]
+
+    return result
+
+
+def _map_image_to_video(metadata: MediaMetadata) -> MediaMetadata:
+    """Map image module metadata field names to video module field names.
+
+    Converts image module field names back to video module field names for
+    compatibility with the video module's exiftool handler.
+
+    @param metadata: Metadata dict with image module field names
+    @return: Metadata with video module field names
+    """
+    # Create reverse mapping: image module field names to video module field names
+    reverse_mapping = {v: k for k, v in _FIELD_MAPPING.items()}
+
+    result: MediaMetadata = {}
+    for key, value in metadata.items():
+        # Use reverse mapped name if available, otherwise keep original name
+        mapped_key = reverse_mapping.get(key, key)
+        result[mapped_key] = value  # type: ignore[literal-required]
+
+    return result
+
+
 def get_xmp_lang_string(value, lang="x-default"):
     lang_key = 'lang="{}"'.format(lang)
     if value and isinstance(value, dict) and value.get(lang_key):
@@ -234,7 +294,17 @@ def write_metadata(input: bytes, metadata: MediaMetadata) -> bytes:
     xmp = {k: v for k, v in xmp.items() if v}
     iptc = convert_xmp_to_iptc(xmp)
 
-    with pyexiv2.ImageData(input) as img:
-        img.modify_xmp(xmp)
-        img.modify_iptc(iptc)
-        return img.get_bytes()
+    try:
+        with pyexiv2.ImageData(input) as img:
+            img.modify_xmp(xmp)
+            img.modify_iptc(iptc)
+            return img.get_bytes()
+    except (RuntimeError, ValueError) as e:
+        logger.warning(f"Failed to write image metadata with pyexiv2: {e}, trying exiftool fallback", exc_info=True)
+        # Try to use exiftool as fallback when pyexiv2 fails
+        # Import here to avoid loading exiftool dependencies unless needed (lazy loading)
+        from superdesk.media.video import write_metadata as write_metadata_video
+
+        # Map image module metadata to video module field names
+        mapped_metadata = _map_image_to_video(metadata)
+        return write_metadata_video(input, mapped_metadata)

tests/media/fixtures/cp-sports.jpg

@@ -21,6 +21,13 @@ def image_binary() -> bytes:
         return f.read()
 
 
+@fixture
+def cp_sports_image_binary() -> bytes:
+    image_path = fixture_path("cp-sports.jpg", "media")
+    with open(image_path, mode="rb") as f:
+        return f.read()
+
+
 def test_picture_metadata_read_write(image_binary) -> None:
     metadata = read_metadata(image_binary)
     assert metadata == MediaMetadata(
@@ -91,3 +98,75 @@ def test_get_metadata_from_item() -> None:
         Title="bar",
         JobId="baz",
     )
+
+
+def test_cp_sports_metadata_write(cp_sports_image_binary) -> None:
+    """Test updating metadata in cp-sports.jpg."""
+    metadata = read_metadata(cp_sports_image_binary)
+    assert metadata["Country"] == "CHN"
+
+    # Update the metadata
+    updated = MediaMetadata(
+        Country="Canada",
+        City="Toronto",
+    )
+
+    next_image = write_metadata(cp_sports_image_binary, updated)
+
+    # Read back and verify the changes
+    metadata = read_metadata(next_image)
+    assert metadata["Country"] == "Canada"
+    assert metadata["City"] == "Toronto"
+
+
+def test_cp_sports_metadata_normalization(cp_sports_image_binary) -> None:
+    """Test that exiftool fallback metadata is normalized to image module field names."""
+    # Read metadata using exiftool fallback (since cp-sports.jpg triggers the fallback)
+    metadata = read_metadata(cp_sports_image_binary)
+
+    # Verify that video module field names are NOT present (should be normalized to image module names)
+    assert "CaptionWriter" not in metadata  # Should be normalized to DescriptionWriter
+    assert "TransmissionReference" not in metadata  # Should be normalized to JobId
+    assert "AuthorsPosition" not in metadata  # Should be normalized to CreatorsJobtitle
+    assert "Rights" not in metadata  # Should be normalized to CopyrightNotice
+    assert "State" not in metadata  # Should be normalized to ProvinceState
+    assert "Credit" not in metadata  # Should be normalized to CreditLine
+
+    # Verify actual metadata values with image module field names
+    assert metadata.get("DescriptionWriter") == "AW"
+    assert metadata.get("JobId") == "RJB101_2022030616"
+    assert metadata.get("CreatorsJobtitle") == "STF"
+    assert metadata.get("CopyrightNotice") == "Copyright 2022 The Associated Press. All rights reserved"
+    assert metadata.get("CreditLine") == "THE CANADIAN PRESS"
+
+
+def test_metadata_field_mapping_consistency(cp_sports_image_binary) -> None:
+    """Test that metadata field mapping denormalization works correctly in write_metadata fallback."""
+    # Create metadata with image module field names
+    metadata_to_write = MediaMetadata(
+        Country="Canada",
+        City="Toronto",
+        DescriptionWriter="updated_writer",
+        JobId="updated_job_123",
+        CreatorsJobtitle="updated_photographer",
+        CopyrightNotice="Updated Copyright",
+        CreditLine="Updated Credit",
+        ProvinceState="ON",
+    )
+
+    # Use cp-sports.jpg which triggers the exiftool fallback for both read and write
+    # This tests that denormalization works correctly in the fallback path
+    updated_image = write_metadata(cp_sports_image_binary, metadata_to_write)
+
+    # Read it back
+    read_back = read_metadata(updated_image)
+
+    # Verify the mapped fields were written and read back correctly through the fallback
+    assert read_back.get("Country") == "Canada"
+    assert read_back.get("City") == "Toronto"
+    assert read_back.get("DescriptionWriter") == "updated_writer"
+    assert read_back.get("JobId") == "updated_job_123"
+    assert read_back.get("CreatorsJobtitle") == "updated_photographer"
+    assert read_back.get("CopyrightNotice") == "Updated Copyright"
+    assert read_back.get("CreditLine") == "Updated Credit"
+    assert read_back.get("ProvinceState") == "ON"