Skip to content
This repository has been archived by the owner on Apr 8, 2024. It is now read-only.

Crash on ios if value of kCGImagePropertyGPSTimeStamp in exif is of type string #188

Open
l0stpenguin opened this issue Aug 22, 2019 · 8 comments

Comments

@l0stpenguin
Copy link

The following code causes spectrum to crash if the gps timestamp property is in string format instead of double. Here is the an extract of GPS dictionary from an image exif:

{
    Altitude = "399.3269045323048";
    DateStamp = "2019:08:22";
    Latitude = "20.224505";
    LatitudeRef = S;
    Longitude = "57.53423333333333";
    LongitudeRef = E;
    TimeStamp = "09:30:04";
}
 CGImageSourceRef source = CGImageSourceCreateWithURL((CFURLRef)[NSURL fileURLWithPath:path], NULL);
    NSMutableDictionary *metadata = [(NSDictionary *) CFBridgingRelease(CGImageSourceCopyPropertiesAtIndex(source, 0, NULL)) mutableCopy];
    FSPEncodeOptions *options =
    [FSPEncodeOptions encodeOptionsWithEncodeRequirement:encodeRequirement
                                         transformations:transformations
                                                metadata:[FSPImageMetadata imageMetadataWithDictionary:metadata]
                                           configuration:nil
                     outputPixelSpecificationRequirement:nil];

If i force the timestamp to a number, it works:

gpsMetadata[(NSString*)kCGImagePropertyGPSTimeStamp] = @0;

the crash happens on this line:
FSPCReportMustFix(@"Unexpected value: %@", value);
from the function FSPInternalRationalsFromValues in FSPImagemetadata.mm

@cuva
Copy link
Contributor

cuva commented Aug 22, 2019

Thanks for reporting this issue @mevinDhun.

The exif spec later on expects the GPS timestamp to be stored as a rationale - i.e a long value - this is the reason why we expect it to be a NSNumber in the exif dictionary.

Can I ask where is this dictionary vended from? I guess we could very well make the conversion slightly less strict and thus allow string to rational conversions. Is this a change you'd be willing to submit?

It'd require adding a else if that checks if the type is NSString and construct the NSNumber from the NSString here.

@l0stpenguin
Copy link
Author

Thanks for the quick response. We have an app where users can pick photos from their roll or via native in app camera capture. So we have seen that many of the photos are causing this crash, they could have originated from third party apps. Here's an example of a plugin which sets the timestamp as string:
https://github.com/bfbat100/react-native-camera/blob/09cdfba96c53251fe8c6a6afde8996b7390abf9a/ios/RCT/NSMutableDictionary%2BImageMetadata.m#L38

While i admit this is a wrong exif, i believe spectrum should not throw an exception in such cases. I currently use the work around to prevent this but i prefer not to handle any exif since spectrum already manages that.

It'd require adding a else if that checks if the type is NSString and construct the NSNumber from the NSString here.

Since it crashes in the isDegrees part, shouldn't the check be added there instead of the last else in the function?
A potential work around to try convert the time string to a timestamp. Not a good solution if the format is not as i specified. A better solution would be if we detect an NSString simply return @0 or {} ?

if (isDegrees) {
    if ([value isKindOfClass:[NSNumber class]]) {
      return FBInternalRationalsFromDecimalAngleValue<T>(value);
    } else if ([value isKindOfClass:[NSString class]]) {
        NSDateFormatter * dateFormatter = [[NSDateFormatter alloc] init];
        [dateFormatter setDateFormat:@"HH:mm:ss"] ;
        NSDate *date = [dateFormatter dateFromString:(NSString*)value];
        NSTimeInterval interval  = [date timeIntervalSince1970];
       return FBInternalRationalsFromDecimalAngleValue<T>(@(interval));
    } else {
      FSPCReportMustFix(@"Unexpected value: %@", value);
      return {};
    }
  } 

@cuva
Copy link
Contributor

cuva commented Aug 23, 2019

Since it crashes in the isDegrees part, shouldn't the check be added there instead of the last else in the function?

Hum - this may be a bug. Both isDegreesEncoded should probably not take into account Entry::GPS_TIME_STAMP given that it ends up being a number.

Right, I was under the impression the timestamp was stored as a number in a NSString - but from the provided code sample it looks like a ISO date in a NSString.

Quick question - have you tried looking though the exif dictionary returned by the platform encoder to see how this entry is formatted? I think that would help us understand what's the best way forward.

Thanks!

@l0stpenguin
Copy link
Author

have you tried looking though the exif dictionary returned by the platform encoder to see how this entry is formatted?

Do you mean what does the exif look like when pictures are taken on stock ios camera app?

@cuva
Copy link
Contributor

cuva commented Aug 23, 2019

Yes that's correct

@cuva
Copy link
Contributor

cuva commented Aug 23, 2019

And if it is an NSNumber, test how the encoder behaves if one provides a NSString formate as an ISO date. Does it drop it or correctly handles it?

I think mapping however the platform encoder behaves natively would make sense - thoughts?

@l0stpenguin
Copy link
Author

I just took an picture from ios stock camera and extracted the exif via exiftool on my mac:

exiftool IMG_1506.HEIC 
File Name                       : IMG_1506.HEIC
GPS Time Stamp                  : 09:51:11.65

So it turns out even Apple use string timestamps. When i select this image from ios image picker in my app, i get this as GPS metadata:

{
    Altitude = "399.2037048358501";
    AltitudeRef = 0;
    DateStamp = "2019:08:26";
    DestBearing = "322.5954897815363";
    DestBearingRef = T;
    HPositioningError = 65;
    ImgDirection = "322.5954897815363";
    ImgDirectionRef = T;
    Latitude = "20.22451333333333";
    LatitudeRef = S;
    Longitude = "57.534225";
    LongitudeRef = E;
    Speed = 0;
    SpeedRef = K;
    TimeStamp = "09:51:11";
}

And hence it crashes spectrum.
Screenshot 2019-08-26 at 14 20 10

@l0stpenguin
Copy link
Author

@cuva any updates on this?

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

No branches or pull requests

2 participants