Fix NDI video stretched #1543
Fix NDI video stretched #1543
Conversation
|
This bug/fix confuses me, so I want to understand what is happening before merging. I won't be able to do that this week |
|
No problem it confuses me too. The only reason I even though to use the stride in bytes is because in ffmpeg::make_frame the width is devided by the pixel stride for some reason. In reality I am thinking about directly creating a casparcg frame from the libndi frame and avoiding the intermediate ffmpeg frame all together but I don't have time right now for such a refactor. This is more a hacky solution that fixes the problem. |
|
If it helps at all. It is similarly broken on windows. Seems like something changed with NDI library sometime? I am reasonably certian this worked fine on 2.4.0 on release on windows since that was what I was running but with whatever NDI version was running then but when I run it now with the latest NDI it is in fact broken on windows as well. The same fix fixes the issue on linux as well, |
|
Some more testing. It work correctly on commit 55e3a04 however at master it does not. It is a pain for me to bisect on my windows PC at home (CEF path fails to apply and AMD GPU BUG so need to apply 2 patches to codebase to get it to just build and run) so tomorrow I will try bisect on the linux box at work and hopefull get you to the exact commit that causes the break. I do suspect there was a change introduced when implementing HDR that broke it and I was running a custom build pre v2.4.0 on windows at work before. |
Once you said it didn't work on windows either, I was wondering about this. Definitely plausible that it could have caused this. I wonder if a few ffmpeg codecs/formats could be affected too |
|
I saw the issue on UYVY and BRGA video from NDI. I'm pretty sure though
that almost everything coming from ffmpeg you be IYUV/YUV420 but it's
possible.
Might be time to look into generating some test videos though. Was thinking
about it but just not sure where we would store that since in repo is
likely not the correct solution.
I would want to look at automating the tests but not sure how we would go
about doing that either. I was thinking of just creating a consumer that
would tie into a test framework but testing compressed video being
decompress means maths so the tests will likely be expensive to run.
…On Fri, 14 Jun 2024 at 14:04, Julian Waller ***@***.***> wrote:
I do suspect there was a change introduced when implementing HDR that
broke it and I was running a custom build pre v2.4.0 on windows at work
before.
Once you said it didn't work on windows either, I was wondering about
this. Definitely plausible that it could have caused this. I wonder if a
few ffmpeg codecs/formats could be affected too
—
Reply to this email directly, view it on GitHub
<#1543 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AC6PQWEMG2O52VFNNBX4G23ZHLL4RAVCNFSM6AAAAABJGOWYB2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRXHA3TONBYGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Looks like the commit that broke the behaviour is e3289ec. I am happy to close this pull request if we can determine that there is actually other issues affecting ffmpeg but as is seen in the offending commit's change. Before we were referencing the linesize in ffmpeg::pixel_format_desc and after that commit we are referencing width. I also found the release v2.4.0 is working correctly. So I cherry picked e4e9ed2 ontop of the v2.4.0 tag and am running that Two things I would like to see but if we want to open issues to make tracking them easier I'm happy to do that:
|
This is to fix a reversion which causes stretching on the ndi feed
|
I would really like to get some automated tests in place as well. After working on other codebases for a long time, it felt really shaky editing shared code without any kind of verification of correctness. In theory it should be pretty easy to get some tests up and running considering the modular architecture, but my knowledge of build- and test tools for C++ are all but non-existing. Ffmpeg has a separate setup script for the tests that download all required assets from a location separated from the repository. That might be a possible way to handle test videos for us as well |
|
We already have various build dependencies being fetched from releases in https://github.com/CasparCG/dependencies/ to avoid baking them into the repository, so it would be easy to add them there and get cmake to download them in the same way as the other dependencies when tests are wanted. I am open to having some tests, but I too don't know much about C++ test tooling. And I think it needs some thought on what the tests should be aiming to cover, especially as a lot of the bits that could do with tests have hardware/system requirements |
There is https://github.com/CasparCG/server/tree/v2.4.x If you look at the version numbers of builds, the On a related note, I should be thinking about tagging 2.4.1 at some point, but I haven't figured out the criteria for doing so. |
|
This won't be needed. The offending commit that introduced the issue is erroneous, and once fixed, it will work globally, just as before. I'll push a fix to ffmpeg::make_frame and friends soon(ish). Hopefully, before next week |
Yes please. If possible, I'd like to have my scaling mode PR and cef PR backported, so I can roll another Debian package... 😎 |
|
PR #1580 will hopefully resolve this issue without the duplicated code |
This is a potential fix for #1542, unfortunately I cannot test on windows right now but it does work on linux with and without alpha and at multiple resolutions. Since I suspect there were no issues on windows I rather used an ifdef to not touch the code that runs on windows.