Update MediaEncoding #613
Update MediaEncoding #613
Conversation
| DisposeLogStream(); | ||
| DisposeLiveStream(); |
There was a problem hiding this comment.
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.
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; | |||
| { | ||
| return GetAvailableEncoder("h264_qsv", defaultEncoder); | ||
| } | ||
| {"qsv", "h264_qsv"}, |
There was a problem hiding this comment.
does this mean one won't be able to utilize hardware-assisted h265?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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) |
| return MimeType; | ||
| } | ||
|
|
||
| return MimeTypes.GetMimeType(outputPath, enableStreamDefault); |
There was a problem hiding this comment.
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. |
|
@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? |
MatMaul
force-pushed
the
mediaencoding-dev
branch
from
c3ef22c
to
730c501
Compare
|
@joshuaboniface should be ok now. |
Remove some duplicate code that were causing warnings
MatMaul
force-pushed
the
mediaencoding-dev
branch
from
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.