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

[CameraRoll] Add assetThumbnail prop #1406

Closed
wants to merge 2 commits into from
Closed

[CameraRoll] Add assetThumbnail prop #1406

wants to merge 2 commits into from

Conversation

Iragne
Copy link
Contributor

@Iragne Iragne commented May 26, 2015

Load Assets from Photo library and a memory warning is send.
Add a property in Image to force the display to use the [asset thumbnail] method

Related Issue #779

Load Assets from Photo library and a memory warning is send.
Add a property in Image to force the display to use the [asset thumbnail] method
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 26, 2015
@brentvatne brentvatne changed the title FIX CRASH [CameraRoll] Add assetThumbnail prop Jun 1, 2015
@Iragne
Copy link
Contributor Author

Iragne commented Jun 10, 2015

any news ?

@bparadie
Copy link

@Iragne Your fix works beautifully for me!

@brentvatne
Copy link
Collaborator

ping @nicklockwood

@@ -188,6 +197,7 @@ var nativeOnlyProps = {
defaultImageSrc: true,
imageTag: true,
contentMode: true,
assetThumb:true,

Choose a reason for hiding this comment

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

Hmm, shouldn't this be assetThumbnail?
I see this below:

RCT_CUSTOM_VIEW_PROPERTY(assetThumbnail, NSString, RCTStaticImage)

@Iragne
Copy link
Contributor Author

Iragne commented Jun 18, 2015

Yes agree with you
Sorry for the error

@brentvatne
Copy link
Collaborator

@Iragne - can you rebase this and squash it into one commit? It looks like it won't be a clean merge

@bparadie
Copy link

@Iragne no worries. What puzzles me is that your code actually worked even with that bug...

@Iragne
Copy link
Contributor Author

Iragne commented Jun 18, 2015

ok @brentvatne

@bparadie
Copy link

@Iragne I was curious whether you'll still get OOM crashes with thumbs and checked my camera roll with more than 200 videos. It ran fine without any problems. I believe for the thumbnail case you do not have to use a serial queue. In other words you could use this version (tested on my iPhone 6):

        if( !thumb ) {
          // ALAssetLibrary API is async and will be multi-threaded. Loading a few full
          // resolution images at once will spike the memory up to store the image data,
          // and might trigger memory warnings and/or OOM crashes.
          // To improve this, process the loaded asset in a serial queue.
          dispatch_async(RCTImageLoaderQueue(), ^{
            // Also make sure the image is released immediately after it's used so it
            // doesn't spike the memory up during the process.

            @autoreleasepool {
              UIImage *image = nil;
              ALAssetRepresentation *representation = [asset defaultRepresentation];
              ALAssetOrientation orientation = [representation orientation];
              image = [UIImage imageWithCGImage:[representation fullResolutionImage] scale:1.0f orientation:(UIImageOrientation)orientation];
              RCTDispatchCallbackOnMainQueue(callback, nil, image);
            }
          });
        } else {
          @autoreleasepool {
            UIImage *image = nil;
            ALAssetRepresentation *representation = [asset defaultRepresentation];
            ALAssetOrientation orientation = [representation orientation];
            image = [UIImage imageWithCGImage:[asset thumbnail] scale:1.0f orientation:(UIImageOrientation)orientation];
            RCTDispatchCallbackOnMainQueue(callback, nil, image);
          }
        }

@brentvatne Would you prefer to have that optimization as a separate PR?

@brentvatne
Copy link
Collaborator

@bparadie - yeah let's move that over to a separate PR! thanks

@Iragne
Copy link
Contributor Author

Iragne commented Jun 22, 2015

Hello sorry complex weekend for me and i didn't have time to work on it.
I create a new PR Too mutch conflics with my version. #1704
@bparadie I don't think it's good to not put the thumb in the queue you have a process to perform the thum creation. I correct the orrientation issue.

@bparadie
Copy link

@Iragne no worries, it was just an experiment. The performance is pretty good even with the serial queue. @brentvatne suggested keeping the optimization separate. May I ask why you think it's not a good idea to use async thumb creation?

@Iragne
Copy link
Contributor Author

Iragne commented Jun 23, 2015

@bparadie thanks if you have other point please let me know.
@brentvatne do you have time to validate it ?

@brentvatne
Copy link
Collaborator

Closing because of the new PR

@brentvatne brentvatne closed this Jun 23, 2015
ayushjainrksh pushed a commit to MLH-Fellowship/react-native that referenced this pull request Jul 2, 2020
* Enbale lint for version 0.25

* Fix lint errors for version 0.25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants