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

Fix 3d mode switching for Wetek HUB on Libreelec #12316

Merged
merged 2 commits into from Jul 17, 2017

Conversation

Memphiz
Copy link
Member

@Memphiz Memphiz commented Jun 17, 2017

This might also fix 3d mode switching for other amlogic boxes. Basically it should for all that don't have ppmgr3d which at least my wetek hub has not under libreelec.

I have no idea where the original ppmgr3d code is currently working. Maybe its an android thingy - i have no idea.

This also fixes 3d to 2d Rendering for my wetek hub which didn't do anything before. (it simply didn't adjust the render rect properly before because it assumed ppmgr3d is there and does its magic i suppose).

This should be "backward" compatible to the fact that whenever ppmgr3d is available - it is used.

This also makes the service.odroidc2-3dautoswitch addon obsolete. (which tried to do a similar thing by using hdmitx0 but just feels not so good as implemented in core like for all other solutions).

@lrusak fyi
@koying maybe some info about the ppmgr3d - is it android?

@koying
Copy link
Contributor

koying commented Jun 18, 2017

I haven't followed Amlcodec at all, so I'll leave it to @peak3d to approve.

ppmgr3d is/was used when the 3D extensions were enabled when building the aml kernel. It's not android-specific.
No clue whether that still exists in recent kernels nor whether the "amhdmitx0" path would work in older kernels.

OTOH, as, basically, kodi + amlcodec is only used on LE, afaik, and obviously LE builds the kernel the way it wants, it doesn't seem to make sense to me to keep an "old way" that won't be used anywhere ;)

@koying koying removed their request for review June 18, 2017 08:01
Copy link
Contributor

@peak3d peak3d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From principle fully Ok, but I really don't understand why LE does not activate ppmgr3D. I mislike to have code in kodi that is already part of drivers:

https://github.com/peak3d/linux/blob/c1_qbuf/drivers/amlogic/ppmgr/Kconfig#L18

@stefansaraev
Copy link
Contributor

because LibreELEC/LibreELEC.tv#1175

@peak3d
Copy link
Contributor

peak3d commented Jun 19, 2017

Do we have an backtrace from driver? There are so many high professionals in LE, I bet it would be easy to one of them to catch the issue in driver and I could help reporting it to AML.
This should not sound picky, but looking for the root cause feels better (without knowing hor far the investigation have already gone)

@koying
Copy link
Contributor

koying commented Jun 19, 2017

People, would you mind doing LE talk in LE, please ;)
What I get is that ppmgr3d is still a thing, but that this Kodi PR introduces a way to do 3D even when it's not enabled, which is pretty good.

+1 from me.

@peak3d
Copy link
Contributor

peak3d commented Jun 19, 2017

@koying so for you its Ok to write code around a bug from driver wich is Open Source?
I would fully agree for binary blob driver, but AML drivers are usually very nice readable.

Aren't there maybe some features in ppmgr3d wich we'll use in the future (there are at least some lines of code in the driver source).

Edit B.t.w you pointed LE in your first post, last chapter as well. If you don't want this name here, we should all follow those golden rules (didn't knew about it so far)

@koying
Copy link
Contributor

koying commented Jun 19, 2017

It's not a matter of naming LE (clearly) but a matter of solving (or not) a LE kernel issue, which is somewhat different ;)
As I said, what I get is that ppmgr3d still exists, and could or could not be enabled, even in LE itself, so it makes sense to support both from a Kodi POV.

@Memphiz
Copy link
Member Author

Memphiz commented Jun 19, 2017

Ok - i wasn't aware of that "simple" solution to activate the ppmgr_3d in the kernel config. Well i did and tried it out. It looks like its heavily broken. I can't even tell where to start :D.

But just looking at Kodis implementation for ppmgr_3d_mode here tells me that the kodi code is unintuitive at the least:

https://github.com/Memphiz/xbmc/blob/93293cf9a1edd70d314fb1c1ecfb9398a850d9ad/xbmc/cores/VideoPlayer/DVDCodecs/Video/AMLCodec.cpp#L2228

  1. it switches dozens mode just for 3d to 2d - which should normally only be a different render rect
  2. When SPLIT_VERTICAL or SPLIT_HORIZONTAL is requested - it DISABLES 3D mode - ohhh yeah that makes sense (whoever coded this should have added a comment imo...)
  3. When doing RENDER_STEREO_MODE_INTERLACED it finally looks like it does something correct and switches to various 3d modes. BUT - there is no option in Kodi to select stereo mode interlaced - so for me this whole code path for RENDER_STEREO_MODE_INTERLACED is dead code.

I tried to manually switch to LR mode by doing "echo 257 > ppmgr_3d_mode" which is hex 0x101 and should be 3d_LR (side by side so to say). The only thing that happened is that the picture freezes and the whole box gets unresponsive. After that i have to reboot the box because killall -9 kodi.bin does nothing.

@Memphiz Memphiz added the WIP PR that is still being worked on label Jun 19, 2017
@Memphiz
Copy link
Member Author

Memphiz commented Jun 19, 2017

added WIP label as i am investigating some strange hdmi handshakes that occure with this patch (even with last commit).

@Memphiz Memphiz added Backport: Needed Type: Fix non-breaking change which fixes an issue Platform: Linux Component: Video rendering v18 Leia and removed WIP PR that is still being worked on labels Jun 19, 2017
@Memphiz
Copy link
Member Author

Memphiz commented Jun 19, 2017

fixed that - was a stupid typo on my side ...

@Memphiz Memphiz added the WIP PR that is still being worked on label Jun 20, 2017
@Memphiz
Copy link
Member Author

Memphiz commented Jun 20, 2017

Added wip label again. Oh man - sleeping another night over this might have revealed a missunderstanding.

  1. this Pr is about adding display mode switching - so my projector or a 3d tv is automatically switched to 3d mode. (So this is more of a hdmi feature i guess)
  2. Thats why i think that code doesn't even belong into the codec but into aml windowing (where refresh rates and resolutions are switched already)
  3. Now that i know that ppmgr means Post Processing Manager i think that whole code in the aml codec for ppmgr_3d_mode is meant to only handle image formatting/scaling. Meaning it can generate 3d interlaced image and for all the 3d 2 2d modes it might do some fancy interpolation or so (where i simply stretch the render width for sbs and the render height for top bottom).

Conclusion:

  1. am adding a feature here that was not there before
  2. that feature has to be moved to aml windowing
  3. ppmgr can stay disabled if someone asks me

@peak3d could this make sense to you too?

@kszaq
Copy link
Contributor

kszaq commented Jun 20, 2017

A small note on kernel-side of things: for me it looks like ppmgr3d is present in 3.14 kernel, disabled by default (in Android kernels), it looks more like a residue from 3.10 code porting.

@Memphiz Memphiz added Type: Feature non-breaking change which adds functionality Type: Improvement non-breaking change which improves existing functionality labels Jun 20, 2017
@Memphiz
Copy link
Member Author

Memphiz commented Jun 21, 2017

Updated... i also already have prepared a backport to krypton if this is accepted.

@Memphiz
Copy link
Member Author

Memphiz commented Jun 24, 2017

@peak3d any idea who maintains this or is entitled to approve or deny this?

@peak3d
Copy link
Contributor

peak3d commented Jun 24, 2017

@Memphiz should be reviewed / agreed by @koying. It feels not correct for me but I have currently no time to evaluate the driver issue. If it's urgent, just press the button, I'll not revert it.

@Memphiz
Copy link
Member Author

Memphiz commented Jun 24, 2017

@peak3d did you read my latest comments? What exactly doesn't feel correct? Ppmgr3d was never able to switch 3d modes on the hdmi channel - that was a miss understanding on my side. Thats why i changed the PR for adding the 3d switching into the windowing where it is for other platforms too (rbpi for example). No its not urgent - just wanted to ensure that someone is there to judge this pr ;)

Copy link
Contributor

@peak3d peak3d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine for me!

@koying
Copy link
Contributor

koying commented Jun 24, 2017

I apologize, I've been confused as well.

Indeed, "amhdmitx0" is the only way to signal 3D on HDMI.
It's like that in SPMC since ages, I should have checked (https://github.com/koying/SPMC/blob/spmc-jarvis/xbmc/utils/AMLUtils.cpp#L43)

ppmgr3d is for frames 3D post-processing, so unrelated.

@Memphiz
Copy link
Member Author

Memphiz commented Jun 24, 2017

Now i feel stupid in re-inventing the wheel here. Damn i should have talked to you about that missing feature on wetek instead of lrusak :( - now i am unsure if i should instead backport the solution you've done in SPMC?!?

@Memphiz
Copy link
Member Author

Memphiz commented Jun 24, 2017

@koying it looks like that feature vanished from your krypton branch though ...

@Memphiz
Copy link
Member Author

Memphiz commented Jun 24, 2017

also you set it from rendering where a SetStereoMode method exists. Who in the team can tell me where exactly the hardware is to be switched to stereo mode ... i somehow feel it has to sit in windowing because we switch resolution and refreshrate there too - but looking ath that renderer code the SetStereoMode call sounds good too - and might also ensure that the mode is switched at the right time during playback start ... this is a bit confusing right now imo.

@koying
Copy link
Contributor

koying commented Jun 24, 2017

Your solution is as good as mine ;)
@popcornmix is probably the one to ping, as I assume he does something similar for RPI.

@MartijnKaijser
Copy link
Member

@popcornmix ping.

@lrusak
Copy link
Contributor

lrusak commented Jul 15, 2017

what is the concensus here?

@peak3d
Copy link
Contributor

peak3d commented Jul 15, 2017

IIRC we should merge it (???)

@MartijnKaijser MartijnKaijser merged commit 84cd8cd into xbmc:master Jul 17, 2017
@MartijnKaijser MartijnKaijser removed the WIP PR that is still being worked on label Jul 17, 2017
@MartijnKaijser MartijnKaijser added this to the L 18.0-alpha1 milestone Jul 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport: Done Component: Video rendering Platform: Linux Type: Feature non-breaking change which adds functionality Type: Fix non-breaking change which fixes an issue Type: Improvement non-breaking change which improves existing functionality v18 Leia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants