Page MenuHomePhabricator

[native] Upload video thumbnails to `keyserver` and persist in serverDB
ClosedPublic

Authored by atul on Aug 29 2022, 5:17 PM.
Tags
None
Referenced Files
F3345801: D4979.id16335.diff
Fri, Nov 22, 7:04 AM
Unknown Object (File)
Mon, Nov 11, 2:51 AM
Unknown Object (File)
Sat, Nov 9, 6:49 AM
Unknown Object (File)
Sat, Nov 2, 1:13 PM
Unknown Object (File)
Sat, Nov 2, 1:13 PM
Unknown Object (File)
Sat, Nov 2, 1:13 PM
Unknown Object (File)
Sat, Nov 2, 1:13 PM
Unknown Object (File)
Sat, Nov 2, 1:11 PM
Subscribers
None

Details

Summary

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.

NOTE: This diff can be landed as-is. The entry's container column will be NULL since there's still plenty of work we need to do in multimediaMessageCreationResponder(...) before we're ready to call assignMedia(...) (or the equivalent) to set the container value. However, it's still fine to add these entries to the uploads table because they'll be swept up by the deleteUnassignedUploads (keyserver/src/deleters) cron job that runs every day at 3:30AM PT (keyserver/src/cron/cron.js).
Test Plan
  1. Send a video message on native (iOS Simulator)
  2. Open uploads table in the serverDB (MariaDB)
  3. 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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul retitled this revision from [native] Upload video message thumbnails to `keyserver` to [native] Upload video message thumbnails to be stored in serverDB.Aug 29 2022, 5:17 PM
atul retitled this revision from [native] Upload video message thumbnails to be stored in serverDB to [native] Upload video message thumbnails to `keyserver` to be persisted in serverDB.
atul retitled this revision from [native] Upload video message thumbnails to `keyserver` to be persisted in serverDB to [native] Upload video thumbnails to `keyserver` to be persisted in serverDB.
atul retitled this revision from [native] Upload video thumbnails to `keyserver` to be persisted in serverDB to [native] Upload video thumbnails to `keyserver` and persist in serverDB.
atul edited the summary of this revision. (Show Details)
atul edited the summary of this revision. (Show Details)
atul requested review of this revision.Aug 29 2022, 5:29 PM

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 ↗(On Diff #16088)

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:

image.png (1×950 px, 245 KB)

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 ↗(On Diff #16088)

It looks like currently, in the database, every piece of media has a loop: false property in the extra column, including images.

image.png (1×2 px, 1 MB)

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 ↗(On Diff #16088)

I tested this code without replaceExtension on this line and instead wrote filename and it seems it works (see the filename column):

image.png (156×1 px, 51 KB)

The only difference is the thumb isn't prefixed to the filename column, but that can be fixed easily.

688–691 ↗(On Diff #16088)

Weird that Prettier wants this on separate lines...it's also ok with it on the same line. I think this looks better?

692 ↗(On Diff #16088)

Why don't we pass the onProgress function here, as we do above?

This revision now requires changes to proceed.Aug 29 2022, 10:16 PM

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.

atul requested review of this revision.Aug 30 2022, 9:19 AM
In D4979#144728, @abosh wrote:

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 ↗(On Diff #16088)

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 ↗(On Diff #16088)

I'm not sure where loop: false is being set.

I know it's because loop should only exist for videos, but what is the point of setting it to undefined

Are you suggesting we should set loop for images here because it happens to be happening somewhere else?

685 ↗(On Diff #16088)

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 ↗(On Diff #16088)

Are you suggesting that I should manually override what prettier suggests here?

692 ↗(On Diff #16088)

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.

abosh added inline comments.
native/input/input-state-container.react.js
654–698 ↗(On Diff #16088)

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 ↗(On Diff #16088)

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 ↗(On Diff #16088)

Yup, the filename for the thumbnail is a JPG when it enters here.

I'll take a look to see if replaceExtension is really necessary or we can just prepend thumb

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 ↗(On Diff #16088)

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 ↗(On Diff #16088)

Ohh, that does make sense.

This revision now requires changes to proceed.Aug 30 2022, 9:37 AM
atul requested review of this revision.Aug 30 2022, 9:43 AM
atul added inline comments.
native/input/input-state-container.react.js
662–665 ↗(On Diff #16088)

But it's not doing anything, so that should be figured out.

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 ↗(On Diff #16088)

I'll make any updated that are necessary in this diff.

It'll either remain or change to thumb_${filename}

688–691 ↗(On Diff #16088)

Hm, well it's remaining on multiple lines for me.

Are you using https://marketplace.visualstudio.com/items?itemName=rvest.vs-code-prettier-eslint

tomek added inline comments.
native/input/input-state-container.react.js
698 ↗(On Diff #16088)

Is it ok that we're ignoring thumbnail upload result?

This revision is now accepted and ready to land.Aug 31 2022, 2:43 AM
native/input/input-state-container.react.js
698 ↗(On Diff #16088)

It's handled in the next diff, D4980

native/input/input-state-container.react.js
698 ↗(On Diff #16088)

Yeah it's used in the next diff

native/input/input-state-container.react.js
685 ↗(On Diff #16088)

I tested this code without replaceExtension on this line and instead wrote filename and it seems it works (see the filename column):

Screen Shot 2022-09-05 at 8.40.35 PM.png (302×1 px, 77 KB)

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:

Screen Shot 2022-09-05 at 8.50.01 PM.png (380×1 px, 118 KB)

Screen Shot 2022-09-05 at 8.50.24 PM.png (438×2 px, 167 KB)

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.