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

[aml] Use amlvideo driver for audio/video sync #9896

Merged
merged 1 commit into from Jun 13, 2016

Conversation

@codesnake
Copy link
Contributor

commented Jun 1, 2016

This PR fixes A/V sync issues in amcodec after master clock removal. Requires CONFIG_V4L_AMLOGIC_VIDEO to be enabled in kernel config.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jun 2, 2016

@mapfau you tested this already, right?

@ghost

This comment has been minimized.

Copy link

commented Jun 2, 2016

Yes, tested the previous version on my odroid - C1 with S805 (after some kernel dts fixes wich are pushed to the hardkernel linux tree).

@codesnake: looks quite well, especially the cleanup and the vfm map changes with respect if deinterlace is present or not. Will try this modified code this eveing again.....

private:
int m_fd;
};

This comment has been minimized.

Copy link
@fritsch

fritsch Jun 2, 2016

Member

We can - we don't have to - think of using either using a namespace here if it's too AML specific or factoring the PosixFile to some Utility class like e.g. the sysfsutils.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Jun 2, 2016

Member

why a namespace? the definition is in the body and is not exposed

This comment has been minimized.

Copy link
@fritsch

fritsch Jun 2, 2016

Member

Jep, that's correct. But as it is not directly decoder related but general I suggested the namespace. I am fine with everything in here. I am happy that @codesnake took the burden and implemented it.

@@ -1657,6 +1678,53 @@ bool CAMLCodec::OpenDecoder(CDVDStreamInfo &hints)
return true;
}

bool CAMLCodec::OpenAmlVideo(const CDVDStreamInfo &hints)
{
PosixFilePtr amlVideoFile = std::make_shared<PosixFile>();

This comment has been minimized.

Copy link
@fritsch

fritsch Jun 2, 2016

Member

I think the shared_ptr is not needed from an architectural pov. It's only used to get auto deleted later on, right?

This comment has been minimized.

Copy link
@codesnake

codesnake Jun 2, 2016

Author Contributor

Yes, right, shared_ptr is used here only for auto-deletion. I took PosixFile class from my ION decoder PR, there was more complex initialization, so I used shared pointer there to avoid gotos or a lot of repeating calls to cleanup parts already initialized in a case of failure.

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

works for me. and works better than the previous version of this patch I had (fixes some mpeg-4 samples)

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Jun 2, 2016

+1
I suggest to wait for reply of mapfau though

@FernetMenta FernetMenta added this to the Krypton 17.0-alpha2 milestone Jun 2, 2016

@louisjoy

This comment has been minimized.

Copy link

commented Jun 2, 2016

Hi Guys,

I have been testing that on both Amlogic S805, S812 and the video is in fast forward not normal mode. I am testing some changes and will let you know. All tests are on Android based builds.

@ghost

This comment has been minimized.

Copy link

commented Jun 2, 2016

Just ready with compiling, seems not to work as the previous (old) PTS change, its stuttering now.

vfm map seems to be ok/identical (default { decoder(0) ppmgr(0) deinterlace(0) amlvideo(0) amvideo})

Not quite sure if its new (have to check with deeper search):

20:17:26 T:3049414656  NOTICE: GL: Using AML render method
20:17:26 T:3049414656  NOTICE: GL: Selecting Single Pass YUV 2 RGB shader
20:17:26 T:3049414656   ERROR: GL: BaseYUV2RGBGLSLShader - unsupported format 19
20:17:26 T:3049414656   ERROR: Previous line repeats 1 times.
20:17:26 T:3049414656 WARNING: CLinuxRendererGLES::UpdateVideoFilter - choosen scaling method 1, is not supported by renderer
20:17:26 T:3049414656  NOTICE: GL: NPOT texture support detected

Tested with the latest kodi master branch with this PR on top

Edit: These errors are the same with the "running" version using the old aml_v4l_sync branch.
(The Shader / renderer are not used here so I would be surprised if this is the reason)

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

@mapfau

most likely (stuttering is) caused by the wait vsync change. make sure adjust refreshrate on start/stop is enabled.

here a moving text sample: https://dav.saraev.ca/pub/samples/burosch1-h264.mpg

@ghost

This comment has been minimized.

Copy link

commented Jun 2, 2016

I'm currently testing with inputstream:

20:38:09 T:2798044144 DEBUG: CRenderManager::Configure - change configuration. 1280x720. display: 1280x720. framerate: 0.00. format: AMLCODEC
.....
20:38:10 T:2798044144 DEBUG: DVDVideoCodecAmlogic: detected new framerate(23.976024), video_rate(4004)

I know that it is not optimal (I have to figure out how to detect the correct framerate on startup), but nevertheless my Monitor is not capable for 23.97 fps. In fact my monitor is running with 50hz wich should be optimal for this video here.

I have a lot of:

20:38:14 T:2798044144   DEBUG: CVideoPlayerVideo::CalcDropRequirement - hurry: 1
20:38:15 T:2940650480   DEBUG: Previous line repeats 8 times.

in my log

@ghost

This comment has been minimized.

Copy link

commented Jun 2, 2016

@stefansaraev or @codesnake : which line of code could I comment out to disable the vsync change? Just to check if this is the reason?

Edit: found the code, will disable and check again.


int openingBracePos = sectionMap.find('{') + 1;
sectionMap = sectionMap.substr(openingBracePos, sectionMap.size() - openingBracePos - 1);
StringUtils::Replace(sectionMap, "(0)", "");

This comment has been minimized.

Copy link
@ghost

ghost Jun 2, 2016

Is it true, that the VFM Map entities always are postfixed with "(0)"? I was not sure at the time I did a similar hack whan trying to get it run without deinterlace driver.

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

@mapfau compare old CRendererAML::RenderUpdateVideoHook with one from this PR

@ghost

This comment has been minimized.

Copy link

commented Jun 2, 2016

@stefansaraev I have uncomened the content of "WaitForVSync", makes the effect much more visible, runs half a second well, then hurries about 10 frames, then again shortly ok and so on.

I'll try to figure out what part of code makes the difference, hpefully I'll find the "old" pts change code of @codesnake

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

here what I used few hours ago:

stefansaraev@512eeaa

EDIT: ah, it could also be buffering thing from what I see, but I am watching livetv via pvr.hts as we speak and no issue here. :)

@ghost

This comment has been minimized.

Copy link

commented Jun 2, 2016

thanx, @stefansaraev . I'll try to figure out something today, cannot guarantee anything because of real life work issues.

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

@mapfau probably unrelated but I think it's worth mentioning

I've been hit by an completely unrelated to amcodec bug after fernet's renderloop rework, that resulted in video stlalls and even deadlocks, I use this workaoround in confluence: https://raw.githubusercontent.com/stefansaraev/TB/master/packages/mediacenter/skin.confluence/patches/0001-hack-render-1x1-pixel-in-fullscreen.patch

it affects only software decoded content here, but who knows..

@ghost

This comment has been minimized.

Copy link

commented Jun 3, 2016

@codesnake it took some time yesterday to rebuild kodi completely to avoid testing on half compiled / broken kodi. And yes, behaviour changed: Now the video is not shown at all, the VideoPlayer turns to a black view but then nothing happens. Interesting on my system is:

root@bigsmall:~# cat /sys/class/vfm/map
default_osd { osd(0) amvideo4osd}
default_ext { vdin0(0) vm}
default_amlvideo2 { vdin1(0) amlvideo2}
default { decoder(1) ppmgr(1) deinterlace(1) amlvideo(1) amvideo}

provider list:
   decoder.h264
   ppmgr
   deinterlace
   amlvideo

receiver list:
   ppmgr
   amlvideo
   ionvideo
   deinterlace
   amvideo
   amvideo4osd
root@bigsmall:~#

the (1) values behind the default items.
I'm quite sure that I had (0) when using the old commit from you.
Any idea?

Edit: Just checked the old code, its also (1) if the video is playing. I looked "after" playing the video earlier and the (0) / (1) seems to be an bool if its active or not.

So, there is any other reason - I'll merge the old commit now and look if the reason has something to do with a change inside kodi

Edit2: Current master with the old commit (https://github.com/mapfau/xbmc/commit/f062c0a674bdde1f0caf133605202b986e70a2a4)
runs well, if @codesnake does not have any idea I'll have to go deeper into it, but because of lack of time this could take some days :-(

@codesnake

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2016

@mapfau What's in Kodi log? I tested on AML-8726MX and S812 and both play well. (1) in VFM map means that provider is being in use.

@ghost

This comment has been minimized.

Copy link

commented Jun 3, 2016

http://paste.ubuntu.com/16942081/
(kodi log contains: start of kodi / select amazon plugin / choose an film / let the film (black) 10 seconds runing / close kodi)

behaviour is:

  • If I restart the system new, and use the new commit, nothing happens (see log)
  • If I let the old commit running, and afterwards kodi with the new commit, its runing but stuttering.

Something seemst to be not OK with the PTS, aml is started regardung the vfm map "(1)"

@codesnake

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2016

@mapfau We just need to make sure we use the same sample(s) to test. Otherwise we will always have different results.

BTW playing a 23.976 video in 50hz display mode you will always have stuttering with or without this PR.

@ghost

This comment has been minimized.

Copy link

commented Jun 3, 2016

Same video is playing very smooth with the old PTS fix from you.
The stuttering is not "a little bit" its heavy (seems like 0.5 secs faster then normal / then wait for sync and thes continues over the whole time.

Should I focus on the video @stefansaraev has posted in an earlier post?
Or should I send you a small sequence of the video I'm currently playing?

But, let me revert the changes step by step, There are not so many places wich are different from both commits. I'll come back with a better issue description soon.....

@codesnake

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2016

@mapfau Previous implementation was completely wrong and doesn't play correctly a lot of videos, so I don't see any sense to return back to it. Does the sample sent by @stefansaraev works for you?

@ghost

This comment has been minimized.

Copy link

commented Jun 3, 2016

@codesnake no, @stefansaraev video is not working. I get the first frame, then nothing happens.

@fritsch

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

Please output the PTS the decoder is returning.

2016-06-03 11:38 GMT+02:00 mapfau notifications@github.com:

@codesnake https://github.com/codesnake no, @stefansaraev
https://github.com/stefansaraev video is not working. I get the first
frame, then nothing happens.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#9896 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABCfHcq7svMD5BafXByEFjkPD6u1WfNXks5qH_YsgaJpZM4IsFBz
.

               Key-ID:     0x1A995A9B
               keyserver: pgp.mit.edu

Fingerprint: 4606 DA19 EC2E 9A0B 0157 C81B DA07 CF63 1A99 5A9B

@ghost

This comment has been minimized.

Copy link

commented Jun 12, 2016

My feedback on odroid C1 (booted before I stared testing!!, and kodi settings are identical to the settings I use with the first PTS commit from @codesnake wich runs still quite good (even if it's wrong):

  • I have no hangs / no blacks and all my videos start, this is fine!
  • the interlaced video from earlier posts in this thread is not smooth at all, the time interval between hurry / wait are smaller. Its better than last commit, but unfortunately far away from "viewable"
  • new are vsync artefacts (stairs in the video) when playing it.

Sorry, and believe me, I would rather give more positive feedback! I really appreciate what you do here, @codesnake.

@codesnake

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2016

@mapfau What interlaced videos you mean? At least one. And on what video I can see stairs?

EDIT: Really interesting about stairs because Kodi AML decoder doesn't really render frames, with this PR or without, rendering is performed by AML driver. This PR only changes the A/V sync mechanism and nothing else. Rendering is performed absolutely the same way as before.

@wrxtasy

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2016

@codesnake
Confirming these should be set for v3.14 S905 ?
Amlogic ion video support
Amlogic ion video support

CONFIG_VIDEOBUF2_ION=y
CONFIG_AMLOGIC_IONVIDEO=y

V4L2 Video Support

CONFIG_V4L_AMLOGIC_VIDEO=y
CONFIG_V4L_AMLOGIC_VIDEO2=y

Also does anyone have a patch to restore the OSD Codec Info window back to Jarvis status so I can see if AML hardware acceleration is being used without referring to logs all the time ?

@ghost

This comment has been minimized.

Copy link

commented Jun 13, 2016

@codesnake
stairs: https://www.dropbox.com/s/4rpwaajt9zrlks3/video.mov?dl=0
still stuttering: https://dav.saraev.ca/pub/samples/burosch1-h264.mpg

I know that there was nothing changed in the rendering pipeline, so I was surprised about the stairs also. I had this effect earlier with the ion approach, and my personal meaning was, that it is an GLES vsync issue. But - with this PR here, its the same effect.
Hopefully there are others reading this with an odroid C1 to verify what I see.
Would be sad, I we try to catch ghosts - I'll build up kodi today evening from scratch.

(My display resolution is 1080p@50hz)

@codesnake

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2016

@mapfau Did you try the same samples in Kodi Jarvis (where amcodec works without issues) and with same(!) kernel? As I said this PR is supposed to fix ONLY A/V sync issue and nothing else.

@codesnake

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2016

@wrxtasy CONFIG_V4L_AMLOGIC_VIDEO=y should be enough.

@ghost

This comment has been minimized.

Copy link

commented Jun 13, 2016

@codesnake I always test both videos with krypton and your first fix regarding PTS.
They run well with this one. Kernel is always the same, I only replace kodi.bin.

More in detail:

  • I have kodi without any of your fixes.
  • I merge the initial PTS commit from you, test if it runs.
  • I reset this comit and pull the one from this PR.
  • After this I boot my machine and test again.

Unfortunately I didn't had any time yesterday to analyze the logs, I'll do again this evening.

@codesnake

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2016

@mapfau:

  1. Why you compare this PR with Kodi Krypton that has broken amcodec support and plays video completely disconnected from audio? If you want to compare, then compare it with Kodi Jarvis that has amcodec fully working.
  2. As I said you numerous times my previous attempt was completely wrong (you refer it as initial PTS commit). I don't understand why you continue to test and compare with it. It uses amvideo driver in freerun mode that mean it doesn't respect PTS and presents frames as fast as it gets them from decoder.
@wrxtasy

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2016

I cannot get smooth video playback at all, running LE 8.x Krypton on the S905 C2 using a patch of this commit 5982883
Debug log:
http://sprunge.us/DeXM

The only extra patches I'm using for Kodi are standard LE Krypton Kodi patches and specific C2 Project AML Audio enabling ones and this:
https://github.com/wrxtasy/LibreELEC.tv.7.0/blob/libreelec-7.0/projects/Odroid_C2/patches/kodi/k10-CEGL-native-type-amlogic.patch

Any other essential v3.14 AML Kernel patches or Kodi AML tweaks needed ?

@codesnake

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2016

@wrxtasy It seems Odroid's 3.14 kernel doesn't have the /sys/module/amvideo/parameters/omx_pts_interval_lower param used in this PR to differentiate kernel 3.14 from older kernel 3.10, but latest kernel on openlinux.amlogic.com does have it. Does Odroid use outdated (pre-release) kernel 3.14?

@wrxtasy

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2016

Yes you're correct i have just checked and that omx_pts_interval_lower param is missing.
I will ask HardKernel directly in their forum. They are usually pretty responsive to implementing essential fixes.

There is only a omx_pts parameter present currently, used a dirty workaround for S905 C2:

if (access("/sys/module/amvideo/parameters/omx_pts", F_OK) != -1)

Report:

  • can only get video playback with Sync Playback to Display Enabled = No HD Audio Passthrough..
  • 23.976fps video is output at 24hz and Nill Frame Rate Automation = video judder every 41 seconds..
  • I will have to investigate YADIFx2 Software deinterlacing again as well for 480/576i DVD's...
@codesnake

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2016

@wrxtasy Really strange, because I never enable Sync Playback To Display and all works.

EDIT: How software decoder and software deinterlacing are related to amcodec?

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2016

they are not related. sync playback to display should never be enabled on aml

are we ready to go? because, if we want to make it "better than in jarvis", it seems, we wont get this merged before christmas...

not that in jarvis it was perfect, but lot of unrelated stuff changed in krypton videoplayer / rendering.

guys, be sure you do NOT test with mpeg-2 samples. I'll merge this in one hour, raise your hand if you disagree (and tell me why)

@wrxtasy

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2016

@stefansaraev I would say its the best I've seen for AML on Krypton at the moment, I can always patch around issues for lagging v3.14 AML Kernels.
Or just revert everything in Krypton master and use the old IVPClockCallback patch you have listed previously to compare the two for video playback, until I sort out this S905.

@stefansaraev stefansaraev reopened this Jun 13, 2016

@stefansaraev stefansaraev merged commit 5334b83 into xbmc:master Jun 13, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@ghost

This comment has been minimized.

Copy link

commented Jun 14, 2016

On odroid C1 there is no /sys/module/amvideo/parameters/omx_pts.
I'll go to figure out what is missing for it in kernel driver.

@wrxtasy

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2016

If I have read Codesnake's code correctly, omx_pts_interval_lower is not needed for the (C1's) v3.10 Kernel that you get in the S8XX's its only required for the (C2's) v3.14 S905 Kernel.
https://github.com/xbmc/xbmc/pull/9896/files#diff-4e99a88003816cb71e489e8a0db42345R1917

@ghost

This comment has been minimized.

Copy link

commented Jun 14, 2016

Should be answered by codesnake, From my understanding omx_pts is the essential key of this development here and has to be present on all platforms:
https://github.com/xbmc/xbmc/pull/9896/files#diff-e39867d73d40423a9182df49306f1c6aR135

lrusak added a commit to LibreELEC/LibreELEC.tv that referenced this pull request Jul 5, 2016
WeTek/Kernel: Fix kernel options to make use of xbmc/xbmc#9896 (#518)
* Update linux.arm.conf

* Update linux.arm.conf
drieschel added a commit to drieschel/LibreELEC.tv that referenced this pull request Jul 12, 2016
sraue added a commit to OpenELEC/OpenELEC.tv that referenced this pull request Sep 16, 2016
projects/WeTek_*/linux: enable CONFIG_V4L_AMLOGIC_VIDEO,CONFIG_V4L_AM…
…LOGIC_VIDEO2,CONFIG_VIDEOBUF2_ION and CONFIG_AMLOGIC_IONVIDEO to support xbmc/xbmc#9896

Signed-off-by: Stephan Raue <stephan@openelec.tv>
Kwiboo added a commit to Kwiboo/LibreELEC.tv that referenced this pull request Oct 18, 2016
Kwiboo added a commit to Kwiboo/LibreELEC.tv that referenced this pull request Oct 19, 2016
Kwiboo added a commit to Kwiboo/LibreELEC.tv that referenced this pull request Oct 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.