-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Update MediaEncoding #613
Update MediaEncoding #613
Conversation
DisposeLogStream(); | ||
DisposeLiveStream(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
care to explain why you swap two disposes? disposing log as the last one looked more correct to me
|
||
TranscodingJob = null; | ||
} | ||
|
||
private void DisposeLogStream() | ||
private void DisposeTranscodingThrottler() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you revert swapping these two methods? it includes pointless diffing clutter IMHO
@@ -21,6 +24,8 @@ public class EncodingHelper | |||
private readonly IMediaEncoder _mediaEncoder; | |||
private readonly IFileSystem _fileSystem; | |||
private readonly ISubtitleEncoder _subtitleEncoder; | |||
// private readonly IApplicationPaths _appPaths; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why'd you want these in?
{ | ||
return GetAvailableEncoder("h264_qsv", defaultEncoder); | ||
} | ||
{"qsv", "h264_qsv"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean one won't be able to utilize hardware-assisted h265
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like random changes again, the result is the same. H265 encoding is not currently supported I think. I'll stop there :-P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think he just simplified the code a lot by using a map instead of a big unreadable multiples if return.
{ | ||
if (IsVaapiSupported(state)) | ||
if (CheckVaapi(state, hwType, encodingOptions)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right even if hwType
is not vaapi
.
IMHO it should be either named something else or applied only when there's really vaapi
requested.
var isUpscaling = request.Height.HasValue && videoStream.Height.HasValue && | ||
request.Height.Value > videoStream.Height.Value && request.Width.HasValue && videoStream.Width.HasValue && | ||
request.Width.Value > videoStream.Width.Value; | ||
return bitrate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you provide a reason why width+height forces one to specify bitrate?..
@@ -1991,7 +2055,7 @@ public string GetInputModifier(EncodingJobInfo state, EncodingOptions encodingOp | |||
{ | |||
if (string.IsNullOrEmpty(requestedUrl)) | |||
{ | |||
requestedUrl = "test." + videoRequest.OutputContainer; | |||
requestedUrl = "test." + videoRequest.Container; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be something randomly generated, rather than just test
which may clash if there are concurrent requests?..
state.SupportedAudioCodecs = supportedAudioCodecsList.ToArray(); | ||
|
||
request.AudioCodec = state.SupportedAudioCodecs.FirstOrDefault(i => _mediaEncoder.CanEncodeToAudioCodec(i)) | ||
?? state.SupportedAudioCodecs.FirstOrDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right... I think this basically says "if we can't find a codec we can encode to, then encode to first", but how can we encode to first if we cannot encode to any? I suppose this should be an error of some kind instead.
} | ||
|
||
var inputChannels = audioStream == null ? 6 : audioStream.Channels ?? 6; | ||
if (inputChannels >= 6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this check?..
return MimeType; | ||
} | ||
|
||
return MimeTypes.GetMimeType(outputPath, enableStreamDefault); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should cache that in MimeType
field
No idea, ask Luke. I did a git checkout from the mentioned commit and fixes the compile errors. As you noticed the code make little sense (check vaapi method...), the most useful part are the comments for the edge cases. |
@MatMaul so you don't want to improve it? Or you just disagree with my questions? I'm fine either way, just want to get clear. |
I am going to remove the first commit and only keep the changes I made myself (some basic cleanups), let's forget about the "update", it didn't seem to introduce any functional change anyway... |
Or take it like that as you want 🙂 your questions make sense, I just don't want to vet Luke random changes line by line, so I am fine with both. |
@MatMaul well, most of the changes made sense to me, so we should probably keep them anyway, and fix more stuff later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's take it as a whole and fix issues later on.
@MatMaul There's a few merge conflicts from the cleanup; the first two look simple enough but the second two I believe touch on your functional changes. Are you able to fix those up? |
c3ef22c
to
730c501
Compare
@joshuaboniface should be ok now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert the change to the taglib-sharp submodule.
Remove some duplicate code that were causing warnings
730c501
to
5f733a8
Compare
Should be ok now. |
Revert back to 197574f for MediaEncoding. This is the last commit including this code in this repo.
This perhaps fixes #482, not sure. The generated CLI for nvenc seems not completely compliant with the ffmpeg wiki so it's perhaps something else. I don't have the hw to test right now.
Remove some duplicate code that were causing warnings.
Cherrypick PRs #198 and #202.