hippware / rn-chat

MIT License
5 stars 0 forks source link

Bug: Processing on thumbnail images takes too long #575

Closed mstidham closed 7 years ago

mstidham commented 7 years ago

Staging Version: 1.27.6 (20) Production Version: 1.27.2 iPhone 7 plus Version: 10.3.1 Wifi

Actual Result: Take Photo during Bot Creation:

  1. Tap Add Photo
  2. Tap Take Photo (take photo)
  3. Tap Use Photo
  4. Stuck at Saving (Averaged 25 sec to 1.25 minutes to save photo)
  5. Tap Create Bot (after saving is finished)
  6. Image thumbnail is not on Bot Profile (Images are there after kill/reload)

image

aksonov commented 7 years ago

Does 1.24.5 contain this issue?

21 апр. 2017 г., в 20:21, mstidham notifications@github.com написал(а):

Staging Version: 1.27.6 (20) Production Version: 1.27.2 iPhone 7 plus Version: 10.3.1 Wifi

Actual Result: Take Photo during Bot Creation:

Tap Add Photo Tap Take Photo (take photo) Tap Use Photo Stuck at Saving (Averaged 25 sec to 1.25 minutes to save photo) Tap Create Bot (after saving is finished) Image thumbnail is not on Bot Profile

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

zavreb commented 7 years ago

@aksonov, @thescurry believes it may be server related. @mstidham can we test this issue on 1.24.5 staging via Test Flight previous builds.

mstidham commented 7 years ago

I tested on Staging Version 1.27.5 with the same results. I do not have a build 1.24.5 available on TestFlight.

bengtan commented 7 years ago

There is no 1.24.5. Possibly @aksonov meant 1.27.5 (which @mstidham has already tested against)

I think this might be a duplicate of #424/#545 ... or if not a duplicate, then it overlaps sufficiently that #424/#545 is obscuring this.

aksonov commented 7 years ago

I can't do nothing here (about speed of loading) - it is just about upload file to S3 server. About empty thumbnail - it seems to be bug (like we had for cover photo) and it is too pity that it was not reported on Friday for 1.27.5

aksonov commented 7 years ago

Posted my thoughts to development slack channel. I can do new client-side workaround but we really need to fix that server-side.

zavreb commented 7 years ago

Cross-posting from @thescurry (hope you don't mind) but:

why not do this @aksonov, @bengtan:

"image processing" placeholder on the client photo grid while we wait for images to process?

zavreb commented 7 years ago

Cross-posting from dev

@aksonov

I think all these tickets were happened because of quite weird current ‘design’ of image upload/displaying. Client/server operate with own kind of URLs for images - starting with ‘tros:’ Sometimes a client receives URL of some image and tries to load it, but fails when image is not ‘processed’ yet on server/S3. Bernard said that it happens because S3 doesn’t have some kind of ‘callbacks’ to let server now when processing is completed. I’ve tried to google it and found some mention of ‘Async’ module (not included by default) that ” is used to make sure that a callback is triggered after the image processing is complete for all images in the request” (https://mohanpalaniappan.com/2016/08/using-aws-lambda-for-image-processing/) @bernard Could you look at it? I believe we really need to have TROS URL only when image is there. For example browsers don’t show image if image URL is not valid, so why our app should do it differently (i.e. retry few times,etc.) ?

bengtan commented 7 years ago

"image processing" placeholder on the client photo grid while we wait for images to process?

It would improve error handling/user friendliness but it doesn't solve the problem of

Image thumbnail is not on Bot Profile (Images are there after kill/reload)

So, yes, we could do it, but we'd have to solve the issue of images not appearing (for a variety of possible reasons).


Re: the other comment:

I believe we really need to have TROS URL only when image is there.

I'm going to talk to @bernardd about it tomorrow.

zavreb commented 7 years ago

Alright @bengtan seems like the ball is in your court. I'll look to you for next steps.

aksonov commented 7 years ago

We talked a lot today with Beng and Bernard about it. Bernard will add some server-side logic to improve this. I've also just added some workaround to avoid loading of newly created bot (so local thumbnails will be not replaced by non-ready-yet server side thumbnails)

bengtan commented 7 years ago

This is the server side ticket:

Request to download image file should wait until post-processing is complete #643 https://github.com/hippware/wocky/issues/643

However, the fix to #545 (which is very similar, or the same) might fix most of this issue, and relegate the remaining to be low priority.

This should be re-assessed after the fix to #545 is deployed.

mstidham commented 7 years ago

Verified on Staging Bot Snake took 25 seconds to save image. Bot Bird took 30 seconds to save image.

zavreb commented 7 years ago

According to #545 on wocky, it is not yet deployed. Keeping it in Ready for QA until then.

bengtan commented 7 years ago

@mstidham:

Bot Snake took 25 seconds to save image. Bot Bird took 30 seconds to save image.

When you said it took 25 or 30 seconds, did you mean that the app said 'Saving...' for 25 or 30 seconds? If not, can you please clarify what that 25 or 30 seconds encompassed?

Also:

How large are the photos 'Bot Snake' and 'Bot Bird'? How many megabytes?

What is the upload speed on your Wifi/Internet connection?

If the photos are large and the upload speed is slow, 20+ seconds may be reasonable.

mstidham commented 7 years ago

In the original ticket I stated that it was stuck at saving for 25 sec to 1.25 minutes. I was just stating that I uploaded 2 images and neither took longer than 30 seconds. I considered this to be reasonable and fixed.

bengtan commented 7 years ago

Quoting @zavreb:

According to #545 on wocky, it is not yet deployed. Keeping it in Ready for QA until then.

I think this comment is misguided. #545 is not waiting on a server side fix. If you read the latest comments on #545, I've explained further over there.

Hence, this ticket is not waiting on a server side fix either.

Additionally, @mstidham clarifies (in the immediately preceding comment) that she considers it fixed (on Staging so far).

zavreb commented 7 years ago

Per discussion, I'll review this one again.

mstidham commented 7 years ago

Verified on Prod

zavreb commented 7 years ago

Verified on Prod. Closing ticket.