Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds new plugin #266

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

DavidReinberger
Copy link

No description provided.

@HaveAGitGat
Copy link
Owner

Fixed some errors so should be working fine now.

@HaveAGitGat HaveAGitGat changed the base branch from master to log_fixes February 8, 2022 02:31
@HaveAGitGat HaveAGitGat changed the base branch from log_fixes to master February 8, 2022 02:31
HaveAGitGat
HaveAGitGat previously approved these changes Feb 8, 2022
@DavidReinberger
Copy link
Author

Hey @HaveAGitGat Sorry for Force pushing after your updates but I've made a lot of changes over the last few dates and did not pushed them... Please have a look at it now.

@DavidReinberger DavidReinberger marked this pull request as draft February 14, 2022 13:07
@DavidReinberger DavidReinberger marked this pull request as ready for review February 18, 2022 16:52
@DavidReinberger
Copy link
Author

@HaveAGitGat Hello, I've finally finalized the plugin and added a new one. Could you take a look at it now please? :) Thanks

@HaveAGitGat
Copy link
Owner

@DavidReinberger sorry missed this. Seems when running the checkPlugins script there are a couple of things missing.

@HaveAGitGat
Copy link
Owner

Thanks, will have a look through this tomorrow.

@HaveAGitGat
Copy link
Owner

Added some fixes from the checkPlugins test.

I've tried Tdarr_Plugin_dt0y_DaveThe0nly_HWACCEL_SINGLE_TRANSCODER and tbh I'm not too sure of the purpose of the plugin as there's a lot going on.

I've run it on a bunch of h264, hevc and vp9 files (default settings) and it only seems to be changing the audio. Even when setting forceHevc it does not seem to do any video transcoding so seems there is a bug in it.

Comment on lines 168 to 170
const isAlreadyProcessed = file.ffProbeData.tags[CUSTOM_TAG] === 'YES';

if (isAlreadyProcessed) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be merged together

const isAlreadyProcessed = file.ffProbeData.tags[CUSTOM_TAG] === 'YES';

if (isAlreadyProcessed) {
response.processFile = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary processFile

}

if (file.fileMedium !== 'video') {
response.processFile = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary processFile

Comment on lines +56 to +57
// eslint-disable-next-line max-len
if (!supportedResolutions.includes(resolution)) throw new Error(`Unsupported Resolution: ${resolution}, supported are only ${supportedResolutions.join(', ')}`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just encase it in brackets, should get rid of the max length issue.

// eslint-disable-next-line max-len
Description: '[Contains built-in filter] Single-pass transcoder to optimize a media file to be used with Plex/Emby etc.',
Version: '1.0',
Tags: 'pre-processing,ffmpeg,hevc,x264,vaapi,nvenc,configurable,audi,video,subtitles',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +1 to +4
const dispositionMap = {
forced: 'forced',
hearing_impaired: 'sdh',
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only used once, can be hardcoded on line 109 and 110.

id: 'Tdarr_Plugin_dt0y_DaveThe0nly_Subtitle_Extractor',
Stage: 'Pre-processing',
Name: 'Extract Subtitles from video',
Type: 'Video',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say this is a Subtitle plugin and not Video

// This takes the first stream as the best stream to derive new ones from
const [
{
index: bestStreamOriginalIndex, channels: bestStreamChannels, codec: bestAudioStreamCodec, ...bestAudioStream
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Objects should have all their properties on new lines

Comment on lines +232 to +242
if (file.fileMedium !== 'video') {
response.processFile = false;
response.infoLog += 'File is not a video. Exiting \n';
return response;
}

if (!file.ffProbeData.streams.length) {
response.processFile = false;
response.infoLog += 'No streams to process!! \n';
return response;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In these cases the file should probably just error out instead of silently fail like it does now.

defaultValue: '.mp4',
inputUI: {
type: 'dropdown',
options: ['.mp4', '.mkv', 'keep'],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep all options on separate lines like you did for the true/false dropdown :D

@HaveAGitGat
Copy link
Owner

FYI will merge this when I've caught up on the new plugins tests and had time to do some for this one, you can see the tests for other plugins here: https://github.com/HaveAGitGat/Tdarr_Plugins/tree/master/tests/Community

Run using npm run test

@HaveAGitGat
Copy link
Owner

Added some more tests and caught a few things on Tdarr_Plugin_dt0y_DaveThe0nly_HWACCEL_SINGLE_TRANSCODER

So first thing like you say is the macos hardware acceleration.

Next thing is the remove commentary track isn't working:

For some reason coming back as processFile: false when a mock commentary track is added.

Maybe you can take a look?

Then will need to add some tests for the other inputs too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants