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

Computer Vision - send next largest image when full is too big #110

Merged
merged 14 commits into from
Sep 25, 2019

Conversation

johnwatkins0
Copy link
Member

This would resolve #101 by providing backup behavior when the full-sized version of an uploaded image is greater than the maximum size accepted by Computer Vision.

Note: This does not intentionally address the "Additional Context" comment on #101 about this update potentially giving time for external image-hosting services to receive the image before the request is sent to Computer Vision. I suggest a separate issue for that. Maybe this work could be done with wp_schedule_single_event instead of in the same pageload as the image upload.

Description of the Change

In the ComputerVision class, this adds a method to provide the URL of a newly uploaded attachment's largest acceptable version.

In most cases -- assuming most WP users don't routinely upload images greater than 4MB -- we would expect the full-size image to work, in which case this update doesn't substantively change any existing plugin behavior. When an uploaded image is greater than 4MB, however, we make a copy of the image's size metadata, sort the sizes from largest to smallest, and use the first size whose file is less than 4MB.

Alternate Designs

A simpler approach would be to rely on the built-in core file sizes ('large', 'medium', etc.), but we don't actually know whether 'large' will be a given site's largest file size below 'full' -- and it is possible to disable core intermediate image sizes entirely -- so this approach uses a simple algorithm to sort images from largest to smallest regardless of how the sizes are named.

Benefits

Within any site that does not have some radically altered way of handling images, Computer Vision will be able to provide data for any uploaded image.

Possible Drawbacks

Calls to the PHP filesize function become expensive when images are hosted externally, so the behavior implemented here should be a fallback and should not run routinely.

Verification Process

Using a site that is reachable over the web -- which is required because Computer Vision accesses the images via a public URL -- upload an image that is larger than 4MB. After the upload is complete, the expected image metadata should have been generated.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

#101

@johnwatkins0 johnwatkins0 self-assigned this Jul 29, 2019
@jeffpaul jeffpaul added this to the 1.4.0 milestone Jul 29, 2019
@jeffpaul jeffpaul added the type:enhancement New feature or request. label Jul 29, 2019
@jeffpaul jeffpaul requested a review from helen July 29, 2019 15:30
/**
* Retrieves the URL of the largest version of an attachment image accepted by the ComputerVision service.
*
* @param string $full_image The path to the full-sized image source file.

Choose a reason for hiding this comment

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

please keep these @param descriptions aligned

@adamsilverstein
Copy link

@johnwatkins0 Overall the code looks good here.

Testing on a temporary public site however, I never saw images above 4m working. I set the threshold at 50% and small images always worked, large images never worked.

Am I missing some step to testing, are you always getting tags for larger images after your PR?

Spin up a site and test: http://poopy.life/create?plugins=https://github.com/10up/classifai/zipball/feature/handle-overlarge-images

@johnwatkins0
Copy link
Member Author

Thanks, @adamsilverstein . I'll see if I can reproduce the issue you're having.

@johnwatkins0
Copy link
Member Author

@adamsilverstein This is fixed now. Seems like I introduced a last-minute bug involving a function parameter. Thanks for testing!

Feel free to use this environment if you get to it in the next 22 hours -- it has the latest on this branch: http://adaptable-wren.w6.poopy.life/ I removed my image processing credentials.

Tested with 23MB image:

Screen Shot 2019-09-05 at 8 52 09 PM

@adamsilverstein
Copy link

@johnwatkins0 Thanks for fixing that up, I'll give it another test (this link should install from this branch: http://poopy.life/create?plugins=https://github.com/10up/classifai/zipball/feature/handle-overlarge-images).

Looks like some merge conflicts have cropped up, can you take a look at those?

@adamsilverstein
Copy link

Generally this is working much better. I received some tagging results on both small and large images. Yea!

I still see an issue that I think is coming from this PR: when I drag and drop an image to upload it (vs clicking 'Select Image' I regularly see an HTTP error message. Oddly the same image uploads fine with the button select method, or sometimes when dragging and dropping, maybe if other uploads are already in progress?

Have you seen this?

image

@johnwatkins0
Copy link
Member Author

Thanks, @adamsilverstein . I will look into that.

@adamsilverstein
Copy link

@johnwatkins0 Thanks! once you resolve the merge conflicts I can re-test; the issue may have been fixed elsewhere.

@johnwatkins0
Copy link
Member Author

@adamsilverstein I've fixed the merged conflicts. I'm not able to reproduce that issue so far, but I might not be following the right steps. Let me know if you still see it.

Copy link

@adamsilverstein adamsilverstein 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!

Copy link

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

@johnwatkins0 can you test on this test site link? i still see the issue when dragging and dropping images to upload them: http://poopy.life/create?plugins=https://github.com/10up/classifai/zipball/feature/handle-overlarge-images

@adamsilverstein
Copy link

adamsilverstein commented Sep 23, 2019

@johnwatkins0

Steps to reproduce the HTTP error issue:

  • Go to Media and Click the Add New button and use this interface in Media:

image

  • The "Add New" page also has the same issue: /wp-admin/media-new.php

  • Drag to upload an image below 5MB in size

Note you get an HTTP error:

image

image

I see this notice in the logs:
PHP Notice: Undefined index: sizes in /plugins/classifai-for-wordpress/includes/Classifai/Providers/Azure/ComputerVision.php on line 96

also see:
PHP Warning: usort() expects parameter 1 to be array, null given in classifai-for-wordpress/includes/Classifai/Providers/Azure/ComputerVision.php on line 147

PHP Warning: Invalid argument supplied for foreach() in classifai-for-wordpress/includes/Classifai/Providers/Azure/ComputerVision.php on line 149

Also testing on my personal site I don't ever see large images getting captioned

Copy link

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

Lookin' good!

Copy link
Contributor

@helen helen left a comment

Choose a reason for hiding this comment

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

One tiny code review change from me, still pending real world testing on my end.

*
* @param int Default 4MB.
*/
return apply_filters( 'classifai_computervision_max_filesize', 4000000 ); // 4MB default.
Copy link
Contributor

Choose a reason for hiding this comment

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

WordPress has some fun built-in constants for file sizes and time units, like MB_IN_BYTES. Would advise using that here for legibility and technical accuracy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Didn't know about MB_IN_BYTES. Updated.

@johnwatkins0 johnwatkins0 merged commit aa3d62b into develop Sep 25, 2019
@johnwatkins0 johnwatkins0 deleted the feature/handle-overlarge-images branch September 25, 2019 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle Full image sizes above 4MB
4 participants