Related comment: https://phab.comm.dev/D14566#404086
Indeed we are discarding processed media and this diff should fix it.
Details
Details
- Reviewers
bartek ashoat - Commits
- rCOMMfbcddf5abbd0: [native] Fix uploading transcoded media
For each file check in the logs if upload uri is correct. If possible download the file on web and verify format or codec.
iOS:
- a downloaded webp image (needs transcoding) -> downloaded as jpg image
- a photo from camera (needs processing)
- a screenshot (no need for processing)
- a screenshot (with forced processing)
- a hevc video from camera (always needs trancoding) -> downloaded video has h264 codec
- a screen recording (always needs trancoding) -> downloaded video has h264 codec
- a downloaded h264 video (no transcoding)
- a downloaded hevc video -> downloaded video has h264 codec
Android:
- a downloaded webp image (needs processing) -> downloaded as jpg image
- a downloaded jpeg image (no processing)
- a photo from camera (always needs processing)
- a screenshot (always needs processing)
- a video from camera (no need for processing)
- a video from camera (with forced processing)
- a downloaded hevc video (needs transcoding) -> downloaded video has h264 codec
- a downloaded h264 video (no transcoding)
- a screen recording (no transcoding)
- a screen recording (forced transcoding)
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
In my opinion, inadequate testing of your changes is the reason our mobile releases have been blocked for the past 2 weeks.
Given the test plan here is as short as in the past two diffs, I worry that you haven't reflected on this.
Can you try to improve your testing going forward? I'd like this to be the final diff you have to submit regarding the video transcoding work.
Here's some concrete advice:
- Always make sure to cover all scenarios that might be affected by your recent work. In this case, you should at least be considering the changes you made in D14512, D14566, and here
- The "Test Plan" field in Phabricator should include a list of all scenarios you've tested, eg. "upload of video that doesn't require transcoding on Android", "upload of image that DOES require transcoding on iOS"
- A good approach is to make a "cartesian product" of a variety of different cases. Here's what I'd suggest here:
- iOS vs. Android
- Video vs. image
- Requires transcoding vs. not
- Screenshot vs. photo
- The testing plan should not only cover making sure that things appear to work. It should also cover that things are working correctly. In this case, it's not enough to verify that the media successfully uploads and is visible, since we also need to cover making sure that transcoding is working as expected
Please also keep this in mind going forward. I think improved testing can help you avoid the sort of errors we've seen over the past couple weeks.
| native/media/media-utils.js | ||
|---|---|---|
| 98 ↗ | (On Diff #47759) | The other declarations are only up here because they are used in the functions defined directly below. It's confusing to include uriAfterProcessing here, far from where it's used. Let's move this declaration down closer to where it's used. |
| 197 ↗ | (On Diff #47759) | Please add a comment here explaining why we need to pass selection.uri here as opposed to uploadURI. That way we can make sure we don't make the same mistake again (referring to what was solved in D14512) |
| 223 ↗ | (On Diff #47759) | Here as well, although in this comment you should say we're just doing it for consistency (since I think in our experience, expo-image-manipulator has no trouble with filesystem URIs) |
| native/media/media-utils.js | ||
|---|---|---|
| 187 ↗ | (On Diff #47759) | It would be helpful to add a comment here explaining that the upload logic requires a filesystem URI, which is why we set uploadURI here |
| 255 ↗ | (On Diff #47759) | It would be helpful to add a comment here explaining that the upload logic requires a filesystem URI |
Comment Actions
It seems like you've updated the test plan to include the string "(image and video that requires and doesn't require transcoding)", which I suppose is an improvement, but very far from what I was looking for.
I want to see that you've tested the whole feature end-to-end, and that there will be no more issues with it.
Can you please re-read my previous comment? I'd like to see a list of 16 scenarios that you've tested, the cartesian product of the 4 cases I listed above.
Comment Actions
Apologies if the update was in progress, but passing back to you. Feel free to re-request review when the test plan has been updated (and performed) as requested above