apache / cordova-plugin-camera

Apache Cordova Plugin camera
https://cordova.apache.org/
Apache License 2.0
965 stars 1.55k forks source link

Android: encodingType is not ignored when sourceType is set with PHOTOLIBRARY or SAVEDPHOTOALBUM #769

Open 5r1m opened 2 years ago

5r1m commented 2 years ago

As per documentation: https://cordova.apache.org/docs/en/10.x/reference/cordova-plugin-camera/#cameraoptions-errata "encodingType" should be ignored when source type is PHOTOLIBRARY or SAVEDPHOTOALBUM. How ever when "encodingType" is set with "Camera.EncodingType.JPEG" and selecting a png picture from album results in unecessary transformation and is not as per the documentation

Screenshot 2021-10-12 at 1 04 47 PM

The reason seems to be unnecessary check for mime type of the choosen file to match the encodingType in options and when not matched it does the transformation: https://github.com/apache/cordova-plugin-camera/blob/ed216ce7148b97ad23316f0530cd9abc2d8d6562/src/android/CameraLauncher.java#L729

Is this a bug or a known behavior? Atleast there is difference in documentation and implementation and there is no option to avoid transformation with out providing the same encoding type which is not possible to provide before hand

breautek commented 2 years ago

Looks like this might have been a regression introduced from https://github.com/apache/cordova-plugin-camera/pull/731

5r1m commented 2 years ago

Thank @breautek for the hint. In such case, can the PR be reverted @PieterVanPoyer ? Also we are seeing weird problems in this use case, like file name changing to png.jpg in pixel devices and modified.jpg in samsung devices (same name for every image being picked) etc. by which its overwriting in temp folder by the last selected picture. So apart from unnecessary conversion, it leading to other issues as well.

PieterVanPoyer commented 2 years ago

Hey @5r1m

I think you are correct. The documentation and implementation are not in sync. Something must be done to fix this mismatch. I checked the iOs version, and that version always transforms to the encodingType. So, I think it would be better to keep this 2 implementations behave the same, and change the documentation.

At this point there is still a difference between these 2 implementations. On Android only next formats are being transformed - https://github.com/apache/cordova-plugin-camera/blob/ed216ce7148b97ad23316f0530cd9abc2d8d6562/src/android/CameraLauncher.java#L790-L791

On iOs, a Gif, a BMP, and more types are being transformed to the encodingType.

Kind regards Pieter

5r1m commented 2 years ago

Hi @PieterVanPoyer

Yes, ideally iOS and Android should be in sync and especially when they are not specific to any platform support/limitation. How ever now the question is

But I think it's not like one thing fits all. In this new version, there is no way to avoid transformation 100% until we know the mime type which user will be selecting well ahead, which is not possible. So instead of forcing the transformation in such cases, why not extend the API to provide options on doing the transformation for selected types only? This way it gives control on transformation to be done or not to API consumers which would be more optimal.

Thanks.

igorsantos07 commented 2 years ago

I know this is almost a year old and the library seems abandoned, but here are my two cents from a fellow dev which is trying (hard) to use the library:

While keeping the conversion would break compatibility, I'd say bump the minor (maybe even the major version, I know it feels harsh but that's what major versions are for) and KEEP the transform! It makes NO sense on:

  1. having a different behavior on iOS when you're coding on a hybrid platform such as Cordova
  2. setting an option which might be ignored depending on another seemingly unrelated option
  3. Lastly, it MIGHT be required by the developer (such as us) since we might be doing image transformation later and we require a fixed image type.

If it is technically possible to convert the picture and always return a given type, why skip that step? When reading the docs, I thought it was a tech limitation, but it looks like it was by design?