-
Notifications
You must be signed in to change notification settings - Fork 538
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
SmartCrop: You have provided a FaceIndex value that exceeds the length of the zero-based detectedFaces array. #133
Comments
@joebernard I came across this. I tried debugging it by breaking down the code and doing some console.logs(). To my surprise, when I did that I found that it did actually manage to log detectedFaces array with one entry in it, and also it managed to log the bounding box. So I think this may be more complicated. It would be interesting if you could try the same experiment and see if you get the same thing.
and this was the output of the console.log calls:
console.log(req.FaceDetails[faceIdx]);
and then the error: I'm not really a node developer, so it might be I was not understanding how promises and await works. This behaviour seemed very strange to me though. Unfortunately at this point I had to forget about using SmartCrop and move on, as the lambda environment upgrade had broken my live site, so I just had to get things working as fast as possible. |
Another important thing to know for the change between v3 & v4 is that v3 (using thumbor rekognition - https://github.com/yu-liang-kono/thumbor_rekognition/blob/master/thumbor_rekognition/__init__.py#L32) detected multiple faces and then adjusted the focus to include them all. v4, on the other hand, just focuses on a single face (defaulting to the first one, unless specified) |
Any updates, or possible solutions to this? We recently updated to v4 as well and relying on smart cropping was very important to us since we used it to automatically crop user's profile pictures. We're getting the exact same error on photos that have have zero faces and it's required us to disable smart cropping for now. This feature is extremely important to us and we'd like to get it resolved soon if possible. Thank you! |
@crpahl Apologies for the inconvenience, I'm looking into the issue right now. For clarification, are you looking for the original image to be returned when zero faces are detected with SmartCrop? |
@hayesry No problem and thank you for the quick response! Yeah exactly that. We're just looking for the original image to be returned. In our case, our users will occasionally upload profile pictures that have no faces in them (such as company logos). |
@crpahl Gotcha! I've added this to our feature request board and will try to have it published in the next update. Really appreciate the feedback! |
FWIW, I'm working on a PR that brings in https://github.com/jwagner/smartcrop.js (MIT licensed, no dependencies) and uses that plus Rekognition for some really nice results. I hope to get that submitted this weekend. I'm using it in production now, and I'm very happy with the quality of the results. |
@hayesry Sorry to ping you on this again. Do you have a rough idea of when this will be published? We're looking into addressing this ourselves as it has a pretty high customer impact for us, but if you think it might be addressed in the near future, we can wait slightly longer. |
@ecaron Hello Eric, It would be really helpful if you could kindly push out the pull requests. Thank you |
Any update on the smartcrop PR? |
The current state on the fork from @ecaron seems to work well at least in our testing |
Sorry, I've been delaying submitting the change for a PR because I haven't reviewed the license tree of the dependency & I haven't written tests yet. Since there's so much interest and this ticket is getting some age on it, I'll put time to this tonight. |
That sounds great. One detail we have changed is to force the image to be PNG (.png() in applyEdits()). Without this webp images don't work because either smartcrop or recognition don't accept webp. |
Another issue that's worth pointing out with ecaron's excellent code is that AWS Recognition has a 5MB upload limit which a large image might hit. It's fairly straightforward to solve for nearly all cases by rescaling what you send to Recognition (it returns relative coordinates anyways). |
From CloudWatch:
Edit: There are conditions where it does work: These two cases should be handled by the lambda. If no face is detected, allow the handler to continue as if |
@joebernard Sorry for the late response and inactive engagement for this issue. This issue is in our backlog, but I'll take a look at this one again. |
can we expect this to be fixed in the next release, waiting for this since July. Also I hope the images will be cached after processing as calls to rekognition are chargeable. |
Hi @beomseoklee, has there been any progress on this? |
We have fixed these issues with smart cropping in v5.2.0. |
I upgraded to v5.2.0. It returns a 400 error with these settings:
No matter what padding value I try it returns the same error. If no padding is passed it does return a face very closely cropped and not really usable. |
Has anyone been able to get @G-Lenz Should I open a separate issue for this? |
Hi @joebernard you can feel free to open a new issue, I have also added the issue to our backlog. Thank you for bringing this to our attention. |
Closing this as the I opened #263 regarding the |
I'm attempting to use the
smartCrop
option:This error is logged in CloudWatch:
It is failing because of this line. It doesn't first check if any faces were detected before it starts defining the bounding box. In my opinion,
smartCrop
should fail gracefully (not crop) if no faces were detected in the image. Would you accept a PR for this, or provide some advisement on how best to implement it?The text was updated successfully, but these errors were encountered: