numbersprotocol / capture-lite

A photo-sharing app with only verifiable photos and videos.
https://numbersprotocol.github.io/
GNU General Public License v3.0
25 stars 6 forks source link

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

Open sync-by-unito[bot] opened 1 year ago

sync-by-unito[bot] commented 1 year ago

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
    1. previous version
    2. No issue
    3. 0.81.3
    4. Just accept less than 4 seconds video, over 5 seconds will display over 30 mb pop-up
  2. Redmi note 10s
    1. previous version (0.79.0)
    2. record 15s short will crash
    3. 0.81.3
    4. Just accept less than 5 seconds video, over 6 seconds will display over 30 mb pop-up
  3. Samsung A21
    1. previous version (0.79.0)
    2. record 15s short sometimes will crash.
    3. 0.81.3
    4. No issue
  4. Redmi Note 11 Pro
    1. previous version (0.79.0)
    2. No issue
    3. 0.81.3
    4. 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[bot] commented 1 year ago

➤ Kenny Hung commented:

Sam (cc Scott YanTammy Yang)

Suggest arranging into 0717 sprint.

sync-by-unito[bot] commented 1 year ago

➤ 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[bot] commented 1 year ago

➤ 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[bot] commented 1 year ago

➤ 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[bot] commented 1 year ago

➤ 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[bot] commented 1 year ago

➤ 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[bot] commented 1 year ago

➤ 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[bot] commented 1 year ago

➤ Sam commented:

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

sync-by-unito[bot] commented 1 year ago

➤ 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[bot] commented 1 year ago

➤ 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[bot] commented 1 year ago

➤ Sam commented:

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

sync-by-unito[bot] commented 1 year ago

➤ 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[bot] commented 1 year ago

➤ 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[bot] commented 1 year ago

➤ 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[bot] commented 1 year ago

➤ 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 ))

sync-by-unito[bot] commented 1 year ago

➤ 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?
sync-by-unito[bot] commented 1 year ago

➤ Tammy Yang commented:

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