Best time to issue PR for Helix

wolfgar commented 10 years ago

Hi, I was wondering what would be the best time to issue a first PR to merge iMX6 upstream.

On the he todo list before inclusion, I think I have to include libfslvpuwrap code in xbmc as we have alluded a few times : I will do it in the coming days...

I am also aware of 2 issues :

HD audio does not work
deinterlacing is jerky @1080

But these two issues are not totally blocking for the inclusion, are they ?

I am looking forward for your feedback, especially you @koying : As a core member you are likely more familiar with this process and the xbmc team expectation...

Best regards Stephan

koying commented 10 years ago

Actually, I was thinking too that it was time to create the actual PR to xbmc, now that we have reached a quite nice level of maturity.

We probably don't even need libfslvpuwrap at this point. IMO, the only relevant thing yet to do is to split our commits into 2-3 logical commits and be sure that non-core tweaks (like https://github.com/xbmc-imx6/xbmc/pull/23 ) are left out...

rabeeh commented 10 years ago

For HD audio stuff; I'm sending CBi4pro to Anssi whom is master in ALSA stuff and hopefully he would be able to contribute to the HD audio effort.

wolfgar commented 10 years ago

Hi Sorry for the delay of my answer : I was not connected this WE...

OK nice ! I will carefully review the diff to ensure non core tweaks are removed and propose a PR if needed @rabeeh : thanks that will probably help ! I had a quick review and found that the driver simply does not deal with HBR : But the hardware should handle it...

wolfgar commented 10 years ago

Just a little update : I have recently bought a AV amplifier hdmi. I was able to give a try at it tonight and I had a closer look at the imx6 sound driver (of course it was so frustrating not to be able to try hd audio with my new toy lol) Good news is: I have found the issue and have just managed to play DTS-HD and dolby truehd with imx6 (for now using unitary tests with aplay) Almost nothing to do : only a little forgotten bit in the original driver... Everything seems to be on driver side... I am preparing a clean patch (which sets this bit when required, for now I have tweaked registers by hand) and will publish it At the end, xbmc imx6 itself is already hd ready I think... ;)

koying commented 10 years ago

Gentlemen, XBMC master and our Gotham-based master have diverged too much to be easily kept in sync in master-pr (especially the 24 bits audio thingy). Could I ask that any further commits be applied to both master and master-pr by the dev, please (besides Gotham merges, obviously).

So master-pr currently lacks 08ed2a15dfa8c21ed41db98ecbee2c4c63397f3a..HEAD form branch gotham-rebased, because it conflicts.

wolfgar commented 10 years ago

Hi chris

I share your concern

I have partly worked on the 24 bit audio thing last week :

First I tried to get rid of all specific 24bit audio work and merge relevant stuff upsteam work (cf this post ) in our master. I have a local branch that compiles but unfortunately my merge may have been a little too blind and fast because I only get static noise
So I tried to check that the 24bit stuff is properly solved upstream by merging this upstream work in our master-pr (and solving the conflicts you are likely speaking about). So far I was hit by a stupid compilation issue of my own because dev branch requires a newer FFMPEG version which includes libswresample I will try to solve this little compilation issue tonight to be able to confirm upstream work is just fine and to be able to update master-pr

Apart from this specific audio stuff, I agree : Let's merge in both branches starting from now as a rule if you prefer...

Regards Stephan

Ryo99 commented 10 years ago

Not sure if this is the right place but please don't forget to have a 13.1 release before advancing.

rabeeh commented 10 years ago

I was wondering if there is an update on this? (PR for upstreaming the code)

koying commented 10 years ago

Hi All,

I've rebased master-pr, removing all "non-core" changes. Basically, it's ready for PR submission.

@wolfgar It's surely best that you create the PR and take merit, as you are the cornerstone of the project, but it's as you see fit :)

koying commented 10 years ago

@sraue @warped-rudi Re OE/Geexbox, it means master-pr lacks the 24bits audio stuff. Not sure how that affects you...

warped-rudi commented 10 years ago

Geexbox is currently using Gotham. But the master changes @FernetMenta did in result of our discussion here should make these patches obsolete.

wolfgar commented 10 years ago

Hi Chris,

Thanks a lot for cleaning up (I planned to do this nwt week as I will be on holidays but you were more efficient, thanks)

Are you sure about 24bits audio stuff ? When I rebased and submitted my pr branch a month ago following your message about merge in pr, I grabbed core 24bits audio stuff which was done by fernetmenta and reverted our specific stuff. In this state, everything was OK. Sorry I have not the thread link at hand but I will add it for reference... ( First there was still a little issue and then Fernetmenta pushed a little fix and it was all good)

I would be glad to submit the PR : If you agree I will initiate it this WE ?

Stephan

koying commented 10 years ago

Well, sure fact there was a bunch of conflicts in the AE code, but I didn't digged in. Anyway, it's probably best to separate PR 's for video and audio stuff. Having video in will be a big win, already.

Re submission, sure, at your leisure. As you planned to work on it anyway, you might want to do some reviewing first...

rabeeh commented 10 years ago

As a side note that shows another build environment for the same code; I'v rebuilt it with latest Android SDK and NDK (thanks koying for the android-r14 tips on xbmc forums).

Here is a link for more description - http://www.solid-run.com/community/topic1091-60.html#p10838

Basically it uses the vpu wrapper instead of MediaCodecs and already showing a tremendous performance boost vs. the ones that uses MediaCodecs.

koying commented 10 years ago

Great! It was meant to work but didn't get time to work on it yet...

sraue commented 10 years ago

@koying added https://github.com/xbmc-imx6/xbmc/pull/87 which fixes a buildproblem and adds (basic) HDMI audio support. just tested master-pr here and for me it looks clean and simple to PR this to xbmc so we have at least basic imx6 support in xbmc (further improvenments should be PR'ed to xbmc after merging)

wolfgar commented 10 years ago

Hi there,

First regarding 24 bits, I have found the thread I told about : https://github.com/xbmc-imx6/xbmc/commit/fd3d7aa53fb2ae45533bd9d65af35adb222898b3#commitcomment-6753474 At that time I had immediately integrated fernetmenta core work on the branch master-pr-test24bit and checked that it worked...So except if there was an issue with merge/rebase, this stuff should be all good

I will review and cleanup last things and issue PR tomorrow or monday ...

wolfgar commented 10 years ago

PR issued : https://github.com/xbmc/xbmc/pull/5202

koying commented 10 years ago

Congratulations :)

sraue commented 10 years ago

thanks much @koying @wolfgar @warped-rudi :-)

rabeeh commented 10 years ago

Congratulations guys for the major milestone !!!

wolfgar commented 10 years ago

@sraue, @rabeeh Thanks a lot ! not a lot of feedback so far but I will be sure to be available to ease this merge as soon as it is required

sraue commented 10 years ago

@wolfgar some feedback we have in this thread: http://www.solid-run.com/community/topic1498.html

esp. with the alsa config solution we cant select spdif: http://www.solid-run.com/community/topic1498.html#p11139 it maybe needs a own config file (i am a alsa noob, so i dont know how to create such config)

but more important to me seems: http://www.solid-run.com/community/topic1498.html#p11127 regarding the subtitles

susisstrolch commented 10 years ago

It looks like the S/PDIF problem is caused by building them as a module - driver load is deferred after the "soundconfig"... I've send you a pull request...

2014-08-14 18:40 GMT+02:00 Stephan Raue notifications@github.com:

@wolfgar https://github.com/wolfgar some feedback we have in this thread: http://www.solid-run.com/community/topic1498.html

esp. with the alsa config solution we cant select spdif: http://www.solid-run.com/community/topic1498.html#p11139 it maybe needs a own config file (i am a alsa noob, so i dont know how to create such config)

but more important to me seems: http://www.solid-run.com/community/topic1498.html#p11127 regarding the subtitles

— Reply to this email directly or view it on GitHub https://github.com/xbmc-imx6/xbmc/issues/67#issuecomment-52208546.

wolfgar commented 10 years ago

@sraue I was speaking about xbmc reviewing in fact.. ;-)

But well, for the points you rightly raise

alsa is an openelec specific issue and susisstrolch seems to have spotted the root cause
subtitles is a long known issue. To be honest I do not know what the cause is but I had forgotten about it, sorry : I immediately create an issue for it #90 not to forget again about it

wolfgar commented 10 years ago

It's merged... Thanks !

xbmc-imx6 / xbmc

Best time to issue PR for Helix #67