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

ffmpegCommandRemoveStreamByProperty transodes to incorrect audio/video codecs in "not includes" mode #562

Open
andrew-kennedy opened this issue Nov 30, 2023 · 10 comments · May be fixed by #688

Comments

@andrew-kennedy
Copy link
Contributor

andrew-kennedy commented Nov 30, 2023

When I transcode any video using the ffmpegCommandRemoveStreamByProperty plugin in "not_includes" mode, it ends up outputting an h264 video with a vorbis audio stream no matter what streams I try to remove. It's very strange.

I was trying to ensure videos I just transcoded to av1 have only an opus audio stream, so I set its property to check to codec_name, the Values to Remove to av1,opus, and the Condition to not_includes mode (implying that I want to remove any streams not in the list), and this ends up always producing a video with h264 video and a single vorbis audio stream, no matter what the input is (even with an input with only av1 and opus audio).

If I switch to "includes" mode, and set the codecs to "aac,ac3,vorbis" (implying that I only want to remove the stream types in the list), the flow plugin properly removes those streams without modifying the remaining streams in any way.

@andrew-kennedy andrew-kennedy changed the title ffmpegCommandRemoveStreamByProperty breaks transcoding in "not includes" mode ffmpegCommandRemoveStreamByProperty transodes to incorrect audio/video codecs in "not includes" mode Dec 7, 2023
@meulop
Copy link

meulop commented Dec 7, 2023

I think the logic for "not_includes" with multiple values is a bit off - effectively it checks each stream against each value in the list in turn, and will remove the stream if any of the tests fail. So in your case I think it will test the video stream and remove because its codec_name isn't "opus", and then will test the audio stream and remove because the codec_name isn't "av1".

@UnknownWitcher
Copy link

UnknownWitcher commented Dec 25, 2023

The issue is that the developer has it looping through the input values and compares each value to each stream property value, so if you have more than one value it results in all streams being removed.

I modified the code and have it placed in my LocalFlowPlugins folder, I don't know how to make the changes on github for @HaveAGitGat to approve it, but I explained in my post how to fix the problem.

@quadcom
Copy link

quadcom commented Sep 11, 2024

The issue is that the developer has it looping through the input values and compares each value to each stream property value, so if you have more than one value it results in all streams being removed.

I modified the code and have it placed in my LocalFlowPlugins folder, I don't know how to make the changes on github for @HaveAGitGat to approve it, but I explained in my post how to fix the problem.

@UnknownWitcher
I'm not quite following on what I need to do to get the fixed version in the LocalFlowPlugins folder. Can you explain a bit more. I'm running in a Docker env btw.

UPDATE: I've edited and re-edited the JS three times carefully following the instructions and I keep getting a read error. I'm messing it up somehow but I can't see where.

What I did notice is your fix is for a different branch. Running off the Master, that file is 105 lines whereas the branch you reference has 103 lines. I've tried to line up your replacement code with the Master branch file but its breaking it somehow. Updating Community Plugins will pull from the Master branch.

Your Link

var prop = String(target).toLowerCase();
for (var i = 0; i < valuesToRemove.length; i += 1) {
var val = valuesToRemove[i].toLowerCase();
var prefix = "Removing stream index ".concat(stream.index, " because ").concat(propertyToCheck, " of ").concat(prop);
if (condition === 'includes' && prop.includes(val)) {
args.jobLog("".concat(prefix, " includes ").concat(val, "\n"));
// eslint-disable-next-line no-param-reassign
stream.removed = true;
}
else if (condition === 'not_includes' && !prop.includes(val)) {
args.jobLog("".concat(prefix, " not_includes ").concat(val, "\n"));
// eslint-disable-next-line no-param-reassign
stream.removed = true;
}
}

Master Branch
https://github.com/HaveAGitGat/Tdarr_Plugins/blob/master/FlowPlugins/CommunityFlowPlugins/ffmpegCommand/ffmpegCommandRemoveStreamByProperty/1.0.0/index.js#L80-L94

@UnknownWitcher
Copy link

@quadcom you can grab this version which has the code I shared to solve the problem. Thank you @rgreen83 for pushing this fix.

Some Instructions:
In your /Plugins/FlowPlugins/LocalFlowPlugins, you want to create the plugin folder with a version slightly higher than the community one, /ffmpegCommandRemoveStreamByProperty/1.0.1, then place the index.js from the mentioned link into that folder, make sure you run a few files through to test it works.

@quadcom
Copy link

quadcom commented Sep 11, 2024

@UnknownWitcher Hey thanks for jumping in.

So I DL'd that file and did what you said. But I kept getting an error in the localFLows section in the Library. Chekcing the container logs and I see the error.

}
[2024-09-11T15:06:40.503] [ERROR] Tdarr_Server - Error: Cannot find module '/app/server/Tdarr/Plugins/FlowPlugins/LocalFlowPlugins/ffmpegCommandRemoveStreamByProperty/1.0.1/index.js/index.js'
Require stack:
- /app/Tdarr_Server/srcug/plugins/noop.js
at Module._resolveFilename (node:internal/modules/cjs/loader:1140:15)
at resolveFileName (/app/Tdarr_Server/node_modules/resolve-from/index.js:29:39)
at resolveFrom (/app/Tdarr_Server/node_modules/resolve-from/index.js:43:9)
at module.exports (/app/Tdarr_Server/node_modules/resolve-from/index.js:46:41)
at module.exports (/app/Tdarr_Server/node_modules/import-fresh/index.js:14:19)
at searchFlowPlugins (/app/Tdarr_Server/srcug/plugins/flowPluginFuncs.js:1:2684)
at async /app/Tdarr_Server/srcug/api/mainApi.js:1:8726{
"code": "MODULE_NOT_FOUND",
"requireStack": [
"/app/Tdarr_Server/srcug/plugins/noop.js"
]

Why it has the path /index.js/index.js makes no sense to me at all!
image

@UnknownWitcher
Copy link

UnknownWitcher commented Sep 11, 2024

@quadcom not sure what's going on with that, in this situation, I'd have to look through @rgreen83 commits to figure out what else they've done unless they know what's going on and could maybe help, however, to get around this, I've uploaded my index.js which I know works (currently using it as we speak).

https://gist.github.com/UnknownWitcher/edee3de3d7504829bf25f410273111a1

Edit: Here's an example of me using it specifically for subtitles, you can select the stream type (All, Video, Audio, Subtitles). In my flow, I use this plugin twice to remove audio/subtitle languages that I don't need.

image

@quadcom
Copy link

quadcom commented Sep 11, 2024

@UnknownWitcher, thanks for sending that up. I checked both files and there is a significant difference between the two. I suspect most of that is due to your addition of the stream types filtering.

Oddly enough, even your version threw up the same error message.

I resolved the error in the most hacky way possible. I created an "index.js" folder inside 1.0.1 and placed the index.js file into it. Basically, I put the file where it said it was looking for it.

This tells me that there is a bug elsewhere in the source code that is not related to this specific issue. It just so happened to show up now.

Anywho, your JS file is recognized, and I will be running some tests.

Thanks again, so very much!!

@quadcom
Copy link

quadcom commented Sep 11, 2024

Well I just filed a bug report as the problem runs deeper.

HaveAGitGat/Tdarr#1078

@UnknownWitcher
Copy link

UnknownWitcher commented Sep 11, 2024

@quadcom I figured out the issue. You left out "ffmpegCommand" in your path, which is why both scripts had an issue.

It should be

/FlowPlugins/LocalFlowPlugins/ffmpegCommand/ffmpegCommandRemoveStreamByProperty/1.0.1/index.js

This was my fault, I forgot about that extra folder. Sorry about that.

@quadcom
Copy link

quadcom commented Sep 12, 2024

That fixed it. Not to worry. I'll kill the bug report.

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