This diff uploads the local thumbnail generated on the native client to the keyserver where the contents will be persisted in the uploads table of the serverDB.
Details
- Send a video message on native (iOS Simulator)
- Open uploads table in the serverDB (MariaDB)
- Check to see whether there are two rows for the sent media message. One with the contents of the video, and another with the thumbnail image:
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Thanks for the NOTE, that helped me understand why this change can be landed as-is! Looks like the SQL query in upload-deleters will catch all these NULL-assigned containers. Some inline suggestions and questions.
native/input/input-state-container.react.js | ||
---|---|---|
654–698 | I don't think the code handling for images/videos (non-thumbnails) needed to be modified if uploadPromises is only scoped to this try / catch block, since we'll need to have access to thumbnailResult in the future anyways. We can just create a variable for it outside where uploadResult is declared, then it's clearer to me what each step means since we explicitly set the inner if statement's result to uploadThumbnailResult. This is a minor change, but it saves some lines so I figured I'd submit it to see if you think it's more concise or clear. Tried doing this in a suggested edit, but it was really hard to see because it was so big: input-state-container.react.js let uploadExceptionMessage, uploadResult, thumbnailUploadResult, mediaMissionResult; try { uploadResult = await this.props.uploadMultimedia( { uri: uploadURI, name: filename, type: mime }, { ...processedMedia.dimensions, loop: processedMedia.mediaType === 'video' ? processedMedia.loop : undefined, }, { onProgress: (percent: number) => this.setProgress( localMessageID, localMediaID, 'uploading', percent, ), uploadBlob: this.uploadBlob, }, ); if (processedMedia.mediaType === 'video') { thumbnailUploadResult = await this.props.uploadMultimedia( { uri: processedMedia.uploadThumbnailURI, name: replaceExtension(`thumb${filename}`, 'jpg'), type: 'image/jpeg', }, { ...processedMedia.dimensions, loop: false }, { uploadBlob: this.uploadBlob }, ); } mediaMissionResult = { success: true }; } | |
662–665 | It looks like currently, in the database, every piece of media has a loop: false property in the extra column, including images. Note the type column being image with the extra column containing a loop: false. Is setting loop to undefined having an effect if loop: false is showing up anyways? Why can't we just set loop to processedMedia.loop? (I know it's because loop should only exist for videos, but what is the point of setting it to undefined if it shows up anyways.) | |
685 | I tested this code without replaceExtension on this line and instead wrote filename and it seems it works (see the filename column): The only difference is the thumb isn't prefixed to the filename column, but that can be fixed easily. | |
688–691 | Weird that Prettier wants this on separate lines...it's also ok with it on the same line. I think this looks better? | |
692 | Why don't we pass the onProgress function here, as we do above? |
We can just create a variable for it outside where uploadResult is declared, then it's clearer to me what each step means since we explicitly set the inner if statement's result to uploadThumbnailResult
Ok, this is my bad, just saw D4980 where you do this. However, the rest of the feedback should still apply. We can still explicitly set each of the calls to uploadMultimedia to uploadResult and uploadThumbnailResult, instead of using Promise.all(...) and destructuring assignment which can be more confusing.
I think it makes sense to use Promise.all(...) so we can kick off both uploads without having to wait for the video to finish uploading before kicking off the thumbnail upload.
native/input/input-state-container.react.js | ||
---|---|---|
654–698 | I think it would be better to kick off both uploads instead of waiting for the video upload to complete before kicking off the thumbnail upload | |
662–665 | I'm not sure where loop: false is being set.
Are you suggesting we should set loop for images here because it happens to be happening somewhere else? | |
685 | Interesting, looks like the filename stuff has changed a bit, I'll take a look to see if replaceExtension is really necessary or we can just prepend thumb | |
688–691 | Are you suggesting that I should manually override what prettier suggests here? | |
692 | We use the onProgress callback to set the progress % so we can display it visually to the user. Right now we're just displaying the progress of video transcoding and video upload. We can re-assess calculating the percentages using both, but in almost every situation (I can imagine) the video will upload well after the thumbnail. |
native/input/input-state-container.react.js | ||
---|---|---|
654–698 | Ah I see. By removing the awaits for each upload result we can decrease some amount of blocking and instead parallelize the uploads. That's pretty smart! | |
662–665 | Ah I worded this badly. I know loop shouldn't be set for images because only videos can loop. I'm saying we should determine where loop is getting set for images, because right now setting it to undefined is not having the intended effect (I think). An undefined prop shouldn't show up in the object in extra, right? And storing the loop property for images is wasteful and sort of inaccurate, which is why we are setting it to undefined in the first place. But it's not doing anything, so that should be figured out. | |
685 | Yup, the filename for the thumbnail is a JPG when it enters here.
Will you update that in this diff or create a separate diff? Requesting changes for a response to this, other than that looks good. | |
688–691 | No...Prettier suggested to put it on one line when I originally wrote the code and hit save. I was noting that it's weird it's also ok with it on separate lines since it auto-formatted it to one line for me. To me, it looks better on one line since that's how it's formatted before (line 660) / takes up less lines. | |
692 | Ohh, that does make sense. |
native/input/input-state-container.react.js | ||
---|---|---|
662–665 |
Agree I'll take a look to see where/why it's still ending up in the extras column but that's out of scope for this diff | |
685 | I'll make any updated that are necessary in this diff. It'll either remain or change to thumb_${filename} | |
688–691 | Hm, well it's remaining on multiple lines for me. Are you using https://marketplace.visualstudio.com/items?itemName=rvest.vs-code-prettier-eslint |
native/input/input-state-container.react.js | ||
---|---|---|
698 | Is it ok that we're ignoring thumbnail upload result? |
native/input/input-state-container.react.js | ||
---|---|---|
698 | Yeah it's used in the next diff |
native/input/input-state-container.react.js | ||
---|---|---|
685 |
If we do just thumb_${filename} the name will end in .mp4 instead of jpg. The reason that it ends up looking right in the database is because of the call to readableFilename(...) made from convertImage(...) in keyserver/src/uploads/media-utils.js: which changes the name (specifically file extension) to match the MIME type of the file contents. Therefore it makes sense to keep this as is (keep using replaceExtension) so that the name is correct all the way through. |