Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added imageSize and playableDuration to include param, deletes isStored #187

Merged
merged 4 commits into from
Jun 23, 2020

Conversation

hsource
Copy link
Contributor

@hsource hsource commented May 23, 2020

Summary

This is another change that is not backwards compatible

I realized that the Android implementation still had poor performance since it looped through individual images to populate image.width/height (some of the time) and image.playableDuration (for every video).

Excluding playableDuration in particular improves performance significantly when the user has videos.

This change:

  • Adds include params for image.width, image.height, and image.playableDuration
  • Refactors Android code to respect those and avoid looping if the params are excluded
  • Makes iOS code respect it too
  • Adds documentation

Side change:

  • This also deletes the useless isStored output that's just a boolean always set to true on iOS

Test Plan

Added toggles to GetPhotosPerformanceExample.

iOS Android
Camera iOS Camera Android

What's required for testing (prerequisites)?

Just the standard yarn start:ios and yarn start:android

What are the steps to reproduce (after prerequisites)?

Compatibility

OS Implemented
iOS
Android

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I updated the typed files (TS and Flow)
  • I added a sample use of the API in the example project (example/App.js)

@bartolkaruza
Copy link
Collaborator

@hsource the other PR is merged, happy to help merging this

@hsource
Copy link
Contributor Author

hsource commented Jun 17, 2020

Thanks! I rebased the change and retested it, and updated the GIFs in the original post! Take a look. It should be ready for review and submission @bartolkaruza

@hsource hsource changed the title Added imageSize and playableDuration to include param Added imageSize and playableDuration to include param, deletes isStored Jun 17, 2020
Copy link
Collaborator

@bartolkaruza bartolkaruza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I only have one question regarding that success boolean returned value.

@bartolkaruza bartolkaruza merged commit ec33d32 into react-native-cameraroll:master Jun 23, 2020
@bartolkaruza
Copy link
Collaborator

Thank you for contributing @hsource !

bartolkaruza pushed a commit that referenced this pull request Jun 23, 2020
# [3.0.0](v2.0.0...v3.0.0) (2020-06-23)

### Features

* Added imageSize and playableDuration to `include` param, deletes isStored ([#187](#187)) ([ec33d32](ec33d32))

### BREAKING CHANGES

* imageSize and playableDuration are no longer included by default to improve performance
@bartolkaruza
Copy link
Collaborator

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

2 participants