facebookincubator / spectrum

A client-side image transcoding library.
https://libspectrum.io
MIT License
1.99k stars 166 forks source link

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

Open l0stpenguin opened 5 years ago

l0stpenguin commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Yes that's correct

cuva commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@cuva any updates on this?