Page MenuHomePhabricator

Format and refactor VideoCutTools worker and add progress bar
Closed, ResolvedPublic

Description

Format worker code with these settings:

{
	"tabWidth": 2,
	"useTabs": true,
	"arrowParens": "avoid",
	"printWidth": 100,
	"singleQuote": true,
	"trailingComma": "none"
}

Refactor code. Some examples:

  • Make less repetitive i.e. cmd.stderr, cmd.on('error' and cmd.on('close' are repeating for video manipulation functions. They need to be added once.
  • Remove unused variables.
  • Change var to let, const as scope appropriate.
  • Rename function like 'callback' to more descriptive names.

Also, add a progress bar to display current video status with bitrate, frame, time and speed data.

Code repos:

Event Timeline

Change 671933 had a related patch set uploaded (by Khr2003; owner: Khr2003):
[labs/tools/video-cut-tool-worker@master] Format and refactor codebase - Changed code to native promise function - Unified reptitive code into single one - Combined audio disable, rotation and cropping into one command - Added prettier and eslint configuration files

https://gerrit.wikimedia.org/r/671933

Khr2003 renamed this task from Format and refactor VideoCutTools worker to Format and refactor VideoCutTools worker and add progress bar.Mar 19 2021, 1:36 PM
Khr2003 updated the task description. (Show Details)

Change 673486 had a related patch set uploaded (by Khr2003; owner: Khr2003):
[labs/tools/VideoCutTool@master] Format and refactor codebase - Changed code to native promise function - Unified reptitive code into single one - Combined audio disable, rotation and cropping into one command - Added prettier and eslint configuration files - Added readme file inside videos folder to add folder to git - Added progress bar to show video process

https://gerrit.wikimedia.org/r/673486

Change 673487 had a related patch set uploaded (by Khr2003; owner: Khr2003):
[labs/tools/video-cut-tool-back-end@master] Added endpoints for progress update

https://gerrit.wikimedia.org/r/673487

Hi @Khr2003! So sweet work! Kudo's \o/!! After trying out locally, I got some feedback to you:

  • Tool's font got affected, see these two images, not just in the header but in other places too, Maybe due to
BeforeAfter
image.png (64×1 px, 11 KB)
image.png (64×1 px, 12 KB)
  • Great progress bar, what does the timing 00:00:16:86 mean? It's not like HH:MM:SS:MS right? Possible to convert into HH:MM:SS:MS formate?

image.png (190×817 px, 63 KB)

  • When the user selects Trim and other settings (Crop, Rotate, Disable Audio). It's better to perform Trip first as the video length will be decreased so it's quicker to apply other options on the resultant video.
  • Also wondering if we can use a single progress bar for all the operations (Trim, Rotate...). Currently, it seems like different progress bars for trim and the rest of the functionalities as it's two different commands I understand.
  • I edited this video locally and the resultant video is too long on the screen as shown in the below image, I suspect because I selected Rotate video too
    image.png (1×918 px, 284 KB)
  • Also wondering if you could document/have comments in the codebase as these are breaking changes.

@Gopavasanth Thanks.

  • I am not sure why the font is affected. I think it is related to dark/light theme toggle and not the progress bar. I can fix that in a separate commit.
  • Timing is the current video time being processed. I think it is in HH:MM:SS.MS format which taken directly from ffmpeg output. I can change it if needed but I think this format is more suitable.
  • I purposely made trim comes after other operations because if there are multiple trims the other operations will cause more performance issues (each trim will be rotated, cropped .. etc, instead of occurring in one step). I did some research to see if trim can be combined with other operations using ffmpeg but could not find any results.
  • Rotation will cause the resultant video to be displayed that way. Nothing to do with the current changes. I can fix this in a separate commit.
  • The changes I added are not breaking. Everything works as it was before with no additional requirement. I only refactored the code. I documented my code as much as possible. I added docblock to each function and comments to where the code seems ambiguous. Otherwise I used clear variable names which should be good indication for future developers.

Awesome!

Timing is the current video time being processed. It is in HH:MM:SS:MS format. What format did you want to change it? The one you suggested is the same as the current one.

Ah yeah, Is it being shown currently when you tried locally?

I purposely made trim comes after other operations because if there are multiple trims the other operations will cause more performance issues (each trim will be rotated, cropped .. etc, instead of occurring in one step). I did some research to see if trim can be combined with other operations using ffmpeg but could not find any results.

Good point, how about having both the methods based on user choices i.e multi trim and single trim, so we may change the rest of the flow for better performance? As if the user trims a video from 1min-10min in a 60min video + Rotate/perform other options, the rest of the encodings are pointless to perform on entire 60min video right and then triming to 10min.

Ah yeah, Is it being shown currently when you tried locally?

I edited my comment before your added the reply.

Good point, how about having both the methods based on user choices i.e multi trim and single trim, so we may change the rest of the flow for better performance? As if the user trims a video from 1min-10min in a 60min video + Rotate/perform other options, the rest of the encodings are pointless to perform on entire 60min video right and then triming to 10min.

That makes sense. I will look into that.

I edited my comment before your added the reply.

No worries, I just saw your edited text. I think it makes sense, agreed.

The changes I added are not breaking. Everything works as it was before with no additional requirement. I only refactored the code.

Ah I was meaning these are really nice and stunning updates xD. Also, sorry I didn't go through the all files in your code changes before, super excited to try out the first so tested the tool locally, just going through your code comments in the code. those are damn great. thank you!

Ah I was meaning these are really nice and stunning updates xD. Also, sorry I didn't go through the all files in your code changes before, super excited to try out the first so tested the tool locally, just going through your code comments in the code. those are damn great. thank you!

Thanks, All good.

I was able to find a way to combine trim with other manipulations. Now if user select trim changes will occur at the same time (trim, rotation, cropping .. etc). I submitted a new patchset.

Change 673486 merged by jenkins-bot:
[labs/tools/VideoCutTool@master] Added progress bar and refactored workers codebase

https://gerrit.wikimedia.org/r/673486

Change 673487 merged by jenkins-bot:
[labs/tools/video-cut-tool-back-end@master] Added endpoints for progress update

https://gerrit.wikimedia.org/r/673487

Change 671933 merged by jenkins-bot:
[labs/tools/video-cut-tool-worker@master] Format and refactor codebase

https://gerrit.wikimedia.org/r/671933

thanks for you help to fix the remainder of the issues. It is awesome to see live after all that work :)