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

[issue] Some device can’t record 15s SHORT by Capture camera. (Not using video uploading.) #2899

Open
sync-by-unito bot opened this issue Jul 11, 2023 · 17 comments
Assignees

Comments

@sync-by-unito
Copy link

sync-by-unito bot commented Jul 11, 2023

User story

As a user, I want the ability to record short videos of up to 15 seconds using the Capture Camera feature.

Reproduce step

  1. Record "SHORT"
  2. enter to preview page
  3. Press next, will display over than 30 MB pop-up

https://app.claap.io/numbers-protocol/video-upload-limitation-issue-c-O35CsUM4Uy-uxaqbTpMYeky

Expectation

User should be able to record 15s SHORT by capture camera

Additional information:

SamComment by @Sam on [Sprint 2] Upload video(SHORT)

Detail test data on android: Comment by @Kenny Hung on [Sprint 2] Upload video(SHORT)

  1. Oppo
  2. previous version
    1. No issue
  3. 0.81.3
    1. Just accept less than 4 seconds video, over 5 seconds will display over 30 mb pop-up
  4. Redmi note 10s
  5. previous version (0.79.0)
    1. record 15s short will crash
  6. 0.81.3
    1. Just accept less than 5 seconds video, over 6 seconds will display over 30 mb pop-up
  7. Samsung A21
  8. previous version (0.79.0)
    1. record 15s short sometimes will crash.
  9. 0.81.3
    1. No issue
  10. Redmi Note 11 Pro
  11. previous version (0.79.0)
    1. No issue
  12. 0.81.3
    1. Just accept less than 12 seconds video, over 13 seconds will display over 30 mb pop-up

┆Issue is synchronized with this Asana task by Unito
┆Created By: Kenny Hung

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Jul 11, 2023

➤ Kenny Hung commented:

Sam (cc Scott YanTammy Yang)

Suggest arranging into 0717 sprint.

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Jul 28, 2023

➤ Kenny Hung commented:

Sam (cc Tammy YangScott Yan)

As our previous huddle, you said this task needs more discussion, could you provide potential solution?

The expectaiton will be

  1. Every user could record 15s SHORT by Capture Camera, no crash.

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Jul 28, 2023

➤ Sam commented:

Kenny Hung, sure.

Potential solution might be upload file in chunks.

Currently we use base64 (it just large text) to upload capture. When we capture image/video

  1. We save image/video in user device as base64
  2. Then we upload to backend as base64

When we read image/video as base64 “object” it loads all data to memory and sometimes crash the app.

So instead of using base64 we can use File and load file data in small parts and upload this small parts lazyly. (Upload file by chunks).

I remember there was a discussiom regarding this before. I will find message and add here as well

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Jul 30, 2023

➤ Tammy Yang commented:

Sam please involve James Chien and Olga for this discussion as it is related to the file accepted in the backend and their integrity

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 2, 2023

➤ Sam commented:

Kenny Hung, for this task I have 2 proposals.

  1. short term
  2. long term and might need some changes from backend

The issue: starting from capture app 0.81.2 we limit asset size registration (client side).

If asset size is more than 30MB ( https://app.asana.com/0/0/1204495833338689/1204888742400415/f ) we just show dialog and prevent asset registrations. This approach save app from crashing when user pick large video/image from gallery. But it will also prevent users from recording videos more than 3-4 seconds (based on user device camera) but camera can record up to 15 second videos.

1st proposal. as short term solution we can do the following.

  1. for video/images picked from gallery, prevent if file size is more than 30MB.
  2. for video/images captured with capture cam, do not prevent at all. This approach might crash app based on user device Comment by @kenny Hung on [Sprint 2] Upload video(SHORT) ( https://app.asana.com/0/0/1204202515424193/1204995997742269/f ), it's not ideal but previous version also didn't limit (prevent) recorded video size and crash app. We just didn't acknowledge that up until recent.

Summary of 1st proposal.

2nd proposal might require some changes in backend.

  1. configure capture app to upload files in chunk (small parts).
  2. configure backend to accept chunk uploads.

Summary of 2nd proposal.

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 2, 2023

➤ James Chien commented:

For 2nd proposal, I think backend could have a endpoint using S3 presigned-url to upload files. This could allow multipart upload and reduce memory footprint, network I/O on backend at the same time.

However implementing it requires some efforts including adding checks to check if the upload completes, some refactoring to avoid duplicate code for create/delete assets, detailed doc to describe how the client side should upload the file. In short this is not a trivial task. So maybe in short term use proposal #1, and in long term plan #2 in later sprints?

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 3, 2023

➤ Olga commented:

Sam I tested streaming uploads to the current storage backend, and it worked fine. Could you attempt not using the base64 object and instead stream the file? This might help resolve the issue.

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 3, 2023

➤ Sam commented:

Olga can you please share the code or postman that does that (streaming upload)?

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 3, 2023

➤ Sam commented:

One thing is that with current implementation Not using base64 might require cascade changes because we use filesystem pluging that works with string only (aka base64)

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 3, 2023

➤ Olga commented:

Sam I wrote a Python ( https://toolbelt.readthedocs.io/en/latest/uploading-data.html#streaming-multipart-data-encoder ) script to accomplish that, but I believe it can be done in other programming languages as well.

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 3, 2023

➤ Sam commented:

Olga thank you for script. Yes I can rewrite that in javascript context. Thank you

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 3, 2023

➤ Sam commented:

Kenny Hung after daily sync we decided to go with proposal 1.

  1. Show "File size exceeded..." dialog for videos/images picked from gallery if larger than 30MB.
  2. No prevent dialog for videos/images recorded with capture cam, actually we had this since 0.63.1 ( https://github.com/numbersprotocol/capture-lite/blob/master/CHANGELOG.md#0631---2022-08-09 ) when we first add custom camera. Now we just need to re-enable it.

Maybe in the future we still need to implement proposal 2 https://docs.google.com/spreadsheets/d/1mKzKcD0LsU_B62kmhuf5RlmtdKMK6NEk2n2dIb-FkNg/edit#gid=1335272736 ( https://docs.google.com/spreadsheets/d/1mKzKcD0LsU_B62kmhuf5RlmtdKMK6NEk2n2dIb-FkNg/edit#gid=1335272736 ). But for now we go with proposal 1.

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 4, 2023

➤ Kenny Hung commented:

Sam (cc Tammy YangScott Yan)

Need to confirm for this release, what is the status for this task?

I try redmi note 10s, when user record 15s video by capture camera, app will crash, it doesn't show the File "File size exceeded..." dialog

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 4, 2023

➤ Sam commented:

Kenny Hung thats correct.

If image/video from gallery → show file exceeded if more than 30 Mb

if image/video from capture cam → show nothing, long videos might crash the app (since capture app 0.63.1)

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Aug 4, 2023

➤ Kenny Hung commented:

Sam comment on milestone. (Comment by @kenny Hung on v230725-capture-app-ionic-internal ( https://app.asana.com/0/0/1205066099827825/1205208537505440/f ))

Copy link
Author

sync-by-unito bot commented Nov 7, 2023

➤ Kenny Hung commented:

Tammy Yang (cc Scott Yan) Regarding this critical issue, needs your confirmation.

  1. "At the current stage, it meets the criteria of the premium plan, and since most users are not using this feature, we can update this issue priority to High.
  2. The issue fixed should be that Capture Cam must not crash within 250MB → [FR] Increase File Upload Size Limit to 250MB ( https://app.asana.com/0/1201016280880500/1204915144119705/f )
  3. If a user subscribes, should we consider allowing Capture Cam to record videos longer than 15 seconds in the future?

Copy link
Author

sync-by-unito bot commented Nov 7, 2023

➤ Tammy Yang commented:

Kenny Hung for big files (larger than 25MB), do not support on App but dashboard.

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

No branches or pull requests

1 participant