Second part of refactoring changes #348
Conversation
New implementation (previously dubbed transcode2()) was judged better than old one. Moreover, removal of old transcode() body allowed me to remove some code from decoder.c as well. What is also important, transmuxer tests had to be fixed: Original code had "modes" here. "Frames" statistic was increased on every video frame when in transcoding mode, and on every video _packet_ when in transmuxing mode. New statistics, for all frame (audio/video) and packet (audio/video/other) types were added and transmuxer tests were modified to use these.
Audio encoder creation was in separate function already, but the video was not. Also, calling everything "output" was confusing, so changed to "encoder".
Past changes have left "strategic" decisions inside workers, and so I moved them up. Also a lot of small changes that helped to converge two separate output creation functions (open_output and reopen_output) into one common function.
MikeIndiaAlpha
requested review from
thomshutt,
emranemran,
AlexKordic and
cyberj0g
MikeIndiaAlpha
force-pushed
the
ma_refactoring_second
branch
2 times, most recently
from
9196c7c
to
cea0459
Compare
Common code isolated out and replaced by functions calls.
Output muxer and output encoders creation logic used to be quite convoluted. This change is an attempt to use muxer and encoders as state variables and avoid separate reasoning upon h->initialized and h->transmuxing. TODO: same on the input side
These changes are aimed at restructuring transcoder init flow, especially on the input side of things. First and foremost, this applies to first half of transcode_init(), which is very similar to open_input (but not 100% same). Also separate open_input() call in lpms_transcode is removed.
Code from transcode_init was moved into lpms_transcode, with the former function deleted. In the latter, code was separated into configuration checking, init, transcode and shutdown parts.
In a way similar to open_input(), also free_input() was sometimes used directly, and sometimes written down manually. So this change aims at producing common way of shutting down the input.
There were separate functions for creating transcoder with or without dnn filters. And separate functions performed init in different ways. To avoid problems in the future, there is single function now, and it creates dnn filters if argument is not null
Trailer writing logic was distributed around the code. In spirit of keeping muxer stuff together it was moved to the output code. Also "keep hardware encoders alive" logic that in case of output relied on two output-closing functions (close_output() and free_output()) was brought in line with how the same works on the input side. Some additional housekeeping wrt filters was done as well. I am not completely happy about hack to keep CountEncodedFrames test from failing, but I expect to return to this problem when tackling audio and video filters. And finally, some code was moved around for more logical grouping, some names were changed, TODOs written, cosmetic stuff.
This is quite deep and important change. Thing is, old code had "demuxer reuse" attempt. And unfortunately it neither made sense (next to nothing performance improvement, complex code, limited to MPEGTS streams), nor was working fine. During Low Latency work I tried to eliminate demuxer reusing because I wanted single demuxer logic to make input I/O plug-in easier. It turned out that so modified code failed certain tests such as Nvidia_AudioOnly. It took me a great deal of debugging, but I found out that there is bug in demuxer reuse procedure: namely only first muxer open attempt _really_ checks what is in the segment provided, the rest just assumes that same streams are there. So basically, in the series of "audio/video"/"audio without video" /"audio/video" segments all will be perceived as containing video! This "patched over" *AudioOnly tests, and let them run, but only because Transcoder "though" that it has video all the time. And this simply wasn't true. When I removed "muxer reusing" code, the Nvidia-dedicated "preserve video decoder" tactics was falling over the fact that pixel format changed (because it changed from AV_PIX_FMT_SOMETHING into AV_PIX_FMT_NONE) and tried to create hardware video decoder for AV_PIX_FMT_NONE, and failed. Fixing all that finally allowed me to get rid of recursive call to open_input when HW decoder needs to be reset. Which is good. During the work on this commit I also noticed that various pieces of code here and there assume that video stream is always there. Which is not true, and never was (for example, some tests were using segments without video in the middle of transcode sequence, and it worked only because Transcoder "failed to notice" there is no video anymore). So the whole code was carefully tested and examined against this dangerous assumption and checks were added. These changes, in turn, allowed to remove the limitations that allowed the Transcoder to start only with segments containing video. Transcoder will now happily process audio-only segments.
MikeIndiaAlpha
force-pushed
the
ma_refactoring_second
branch
from
cea0459
to
c712137
Compare
There was a problem hiding this comment.
LGTM overall, great work @MikeIndiaAlpha!
It's a bit too large to review efficiently, hope we'll be able to make PRs in this part smaller from now on. I ran the tests on Linux x64 + Nvidia locally, hope someone could do it on Darwin as well (just in case, there shouldn't be any platform-related issues).
Minor nitpick, please reword or remove TODOs and pronounces from comments, since it's open source and visible for anyone. Maybe move some of longer and more useful explanations elsewhere. Options, which we used to store additional code-related details before, are:
- Self-review comments in the PR
- QUIRKS.md file in this repo
- Github issues with appropriate tags
Thanks! I hope it will be downhill from now on.
You mean macOS, right? My primary development machine is MBP with Monterey and so I am testing on it. Unfortunately it is positively ancient, 2015 model so Intel not Apple silicon. But as you say, there shouldn't be platform related issues wrt my changes. Also tested on WSL2 on my other machine with Nvidia card.
This is just the way I work, but you're right, it doesn't look pretty. Not that spaghetti code looks pretty :P |
|
Hey, what is the status of this? |
|
@chrishobcroft it's an important piece of work addressing some of the tech debt in |
|
Hey, @MikeIndiaAlpha is there some way I can work with you to get this merged? |
|
@chrishobcroft Sorry, I did this when I was employed by Livepeer and working on LPMS refactoring was my duty. I left the Livepeer around August last year, so you have to work with Livepeer team and not me. Good luck! |
|
Closing due to #410 |
This set of changes is about two things: