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

Audio dsp addon handling #4402

Merged
merged 16 commits into from Jul 19, 2015

Conversation

Projects
None yet
@AlwinEsch
Copy link
Member

commented Mar 12, 2014

Attached a audio DSP processing system over add-ons.

This is my first version and not complete finished. Can you have a look over it and any ideas for things which must be improved or are wrong.

The current system in steps:

  • Data is passed from CActiveAEBufferPoolResample to the DSP processing system if DSP enabled and minimum one add-on is available.
  • All Add-ons are asked about the requested stream type to find available addons and modes, e.g. 5.1 Audio not need a stereo up mix.
  • The system makes a list of master processing modes which are selectable from user.
  • The system makes also a list of pre and post processing modes which are selectable and process point moveable inside Settings->System->Audio->Audio DSP Settings
  • On data processing it goes over following steps:
    1. Input processing - Unmodified input stream and is send to all enabled add-ons for detection and error correction
    2. Input re-sample - Re sample of the input signal for the master processing, only one add-on is allowed for it.
    3. Pre processing - Used for any steps before master processing. All enabled add-ons functions are called to make this.
    4. Master processing - The main processing like, surround up mix or sound modification processes. Only one from user over menu selectable mode is allowed. Add-ons can pass multiple selectable modes to KODI. If input channel alignment on it is higher as requested output and the master mode does not perform a down mix it becomes handled from ffmpeg re sample after return of master function.
    5. Post processing - Used for any other steps like, volume correction, equalizer and much more. All enabled add-ons functions are called to make this.
    6. Output re-sample - Re sampling of the processing signal to KODI audio output processing.

The pre and post processing modes can be selected and moved within processing chain on audio dsp settings inside Settings->System->Audio.

Things to do or finished:

  • The first version of dsp add-on headers and the basic system is finished.
  • Things for me which missing and are to-do:
    1. Code cleanup
    2. Faults removal
    3. Create a helper documentation about a DSP add-on programming
    4. Code re check

Addon position
audiodsp1
The DSP enable position
audiodsp - addon setting6
The button position to select streaming DSP settings dialogue
audiodsp - addon setting4
The basic settings dialogue, separated in sub menus
audiodsp - addon setting5
A add-on master mode settings dialogue
audiodsp - addon setting
Process chain information dialog (with CPU usage)
audiodsp - addon setting2
Add-on mode process chain selection dialogue
audiodsp - addon setting3

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Mar 12, 2014

awesome! and welcome back.

@un1versal

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2014

On that last screenshot, typo on front/rear seperation
is that not supposed to beFront/rear separation.

@AlwinEsch

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2014

The typo is fixed on add-on and have replaced the image.

@AlwinEsch AlwinEsch closed this Mar 15, 2014

@AlwinEsch AlwinEsch reopened this Mar 15, 2014

@t-nelson t-nelson added the Helix label Mar 28, 2014

@AlwinEsch

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2014

The first version of dsp add-on headers is finished from my side. Only thing to-do now is to add more separate AE_DSP_PROFILE types.

Also to-do from add-on side is to add more callbacks to XBMC.

On XBMC the dsp handling it have nearly all needed function and classes, but locks still a bit bad and need a lot of rework, clean up and improvement.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2014

@AlwinEsch: Would it be possible to rebase this on to master so that it builds?

@AchimTuran

This comment has been minimized.

Copy link
Member

commented Apr 7, 2014

@MilhouseVH: If you want you can compile it from alwins fork and select the "audio-dsp-addon-handling" branch (https://github.com/AlwinEsch/xbmc/tree/audio-dsp-addon-handling)

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2014

I'd like to compile an experimental OpenELEC build, but it's usually a positive sign if the PR compiles.

@AchimTuran

This comment has been minimized.

Copy link
Member

commented Apr 20, 2014

I hope this is correct and helps you. Just fork AlwinEsch's xbmc repo to your own account and push the audio-dsp-addon-handling branch to your master branch.

@AlwinEsch

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2014

Hello,

one question about the way to get access on ffmpeg to become detailed stream information about AC3/EAC3, DTS... which can be given then to the DSP add-on's.

Have removed my previous way and searching another one. One other way I see is to access on "AVCodecContext" the "priv_data" field where as example the "AC3DecodeContext" data is stored.

Gives it from ffmpeg and XBMC side a way to have access to this data?

Thanks a lot for help,

Alwin :)

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Apr 22, 2014

I don't think accessing priv_data is a good idea. could you ask this question to the ffmpeg mailing list. I am sure the devs there have an idea of how to do this best.

@davilla

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2014

there's a reason it's called 'private' :)

@AlwinEsch

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2014

Sorry, you right, sometimes I'm a bit stupid :)

Have found a way under the ffmpeg on github, there is a new function system with
"AVFrameSideData *av_frame_get_side_data(const AVFrame *frame, enum AVFrameSideDataType type)"

In it becomes the parts stored which are relevant from my side to the dsp system, e.g. AV_MATRIX_ENCODING_DOLBYEX, AV_DOWNMIX_TYPE_DPLII, surround_mix_level ...

@FernetMenta

This comment has been minimized.

Copy link
Member

commented May 1, 2014

@AlwinEsch could you please squash commits for review. in particular the interface to the current code should be in separated commits.

@AlwinEsch

This comment has been minimized.

Copy link
Member Author

commented May 1, 2014

You mean to correct last commits or for the next ones? If it must corrected I must find out how :), Sorry about my commits.

One another question, the dsp system is currently only be supported over addon's, make it sense to have it also availabe inside XBMC maybe over ffmpeg? But have seen on the latest ffmpeg version there is still not much to use.

If it becomes used I see two ways todo it, one is to rework the interface to use ActiveAEDSPAddon based upon virtual functions with use of a parent class e.g. IAEDSPProcess and internal ones also based on it or the other way to use something on the ffmpeg resample class after master process call (currently on dsp only used for downmix).

  • The problems I see are on first, a bigger change and call hierachy becomes bigger with a bit more CPU usage.
  • For the second is limited to master

Greetings from me from Pantelleria.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented May 1, 2014

I mean the entire pr, 155 commits are impossible to review. You should squash commits like "fix ..." first into others. Or rebuild the commits from scratch, whatever is more convenient to you.

I need to get familiar with this before I can discuss questions like you asked above.

@AlwinEsch

This comment has been minimized.

Copy link
Member Author

commented May 1, 2014

Ah, sorry now understand. I rebuild the commits from scratch in the next days. What me a bit confuse is, if update on my branch with the master, all my previous commits are listed again. That make also a so big list.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented May 1, 2014

what do you consider "update with master"? In general you do git fetch upstream, git rebase [-i] upstream/master

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented May 2, 2014

if there are any commits for cleanup/fix you may also already send them as separate PRs so the final one is smaller

@MartijnKaijser MartijnKaijser added this to the Pending for inclusion milestone May 21, 2014

@AlwinEsch

This comment has been minimized.

Copy link
Member Author

commented May 25, 2014

The complete pull request is cleaned up and have now only 14 commits.

@AlwinEsch

This comment has been minimized.

Copy link
Member Author

commented May 25, 2014

@da-anda About the German translation, must it be removed for the pull request? If not is no problem for me if it becomes overwritten.

@da-anda

This comment has been minimized.

Copy link
Member

commented May 25, 2014

@AlwinEsch AFAIK yes, we only commit english strings via PRs. But keep the file somewhere and I'll add the translations on transifex once this PR is in.

@AlwinEsch

This comment has been minimized.

Copy link
Member Author

commented May 25, 2014

How can I handle changes on the patches. without set your comments out of it? Currently I know only to reset my commits and commit it corrected again?

@AlwinEsch AlwinEsch closed this May 25, 2014

@AlwinEsch AlwinEsch reopened this May 25, 2014

@da-anda

This comment has been minimized.

Copy link
Member

commented May 25, 2014

I'm not 100% sure, but AFAIK if git hashes don't change during rebase, the comments on those commits should persist. If you squashed something, comments on these commits will be lost (new hash). Sometimes github is showing a hint that user X has commented on an outdated commit and you can still go to that commit and read the comments - but I'm not sure if this is always the case. So to be absolutely sure to keep all comments you'd have to append fixup commits and only squash those before PR is being merged - but this might make reviews difficult again.

@AchimTuran

This comment has been minimized.

Copy link
Member

commented Jul 19, 2015

jenkins build this please

@AchimTuran AchimTuran force-pushed the AlwinEsch:audio-dsp-addon-handling branch from 2b7c4c5 to b21eece Jul 19, 2015

@AchimTuran AchimTuran force-pushed the AlwinEsch:audio-dsp-addon-handling branch from b21eece to bad8efc Jul 19, 2015

@AchimTuran

This comment has been minimized.

Copy link
Member

commented Jul 19, 2015

@MartijnKaijser and others
From my side all is done and after the jenkins build succeeded, I will merge AudioDSP if it is ok for you all.

Many thanks to all, who helped and made this possible.

jenkins build this please

AchimTuran added a commit that referenced this pull request Jul 19, 2015

@AchimTuran AchimTuran merged commit 4730434 into xbmc:master Jul 19, 2015

1 check failed

default Merged build finished.
Details

@hudokkow hudokkow added this to the J**** 16.0-alpha1 milestone Jul 19, 2015

@un1versal

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2015

@AchimTuran @AlwinEsch See forum.http://kodi.tv/showthread.php?tid=232628 and congratulations finally :)

@@ -53,6 +53,655 @@ namespace INFO
class InfoSingle;
}

// conditions for window retrieval

This comment has been minimized.

Copy link
@MartijnKaijser

MartijnKaijser Jul 20, 2015

Member

@Paxxi @Wisler this should have not been merged in again as it was removed already in f4daa86

@AchimTuran

This comment has been minimized.

Copy link
Member

commented Jul 20, 2015

jenkins build addons please

@wsnipex

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

this doesn't work on closed/merged PRs

@un1versal

This comment has been minimized.

Copy link
Contributor

commented on xbmc/Application.cpp in 161e359 Jul 22, 2015

RegisterReceveiver -> really?

RegisterReceiver

This comment has been minimized.

Copy link
Member Author

replied Jul 22, 2015

You're right. There is a spelling mistake in the method. See xbmc/xbmc/messaging/ApplicationMessenger.h

void RegisterReceveiver(IMessageTarget* target);

Sorry also in the comments is a mistake.

This comment has been minimized.

Copy link
Contributor

replied Jul 22, 2015

comments I dont really care but fixed in #7584 Ill update comments

@AlwinEsch AlwinEsch deleted the AlwinEsch:audio-dsp-addon-handling branch Sep 11, 2015

@FernetMenta

This comment has been minimized.

@AlwinEsch @AchimTuran
since ADSP is not directly connected to a driver this comment does not make much sense.
I am wondering how an ADSP addon can be flushed by the engine.

This comment has been minimized.

Copy link
Member Author

replied Apr 21, 2016

Yes you are right.

What do you think about:

      /*!>
       * Is the used time as a seconds that are required by the add-ons to processing the data.
       * @return seconds
       */

This comment has been minimized.

Copy link
Member

replied Apr 21, 2016

ffmpeg swresample swr_get_delay:

Gets the delay the next input sample will experience relative to the next output sample

What about flush?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.