Skip to content

Ae final rebase #939

Merged
merged 38 commits into from May 9, 2012
@jmarshallnz
Team Kodi member

This is a rebased version of the old #900. There is no difference other than this one is rebased.

This should be the one to review. I shall keep the previous branch (ae_rebase) open for now while @gnif and @DDDamian et. al. work on DSPs and DRC.

Review is needed:

  1. For build system changes (@theuni, @amejia1 ?)
  2. For dvdplayer changes (@elupus ?)
  3. Any platform specific stuff.
  4. General overview of AE (if anyone has time!)

The main feature dropped currently with this is the volume amplification - gnif and DDDamian are working on a DSP system which should allow this back in later. Note that this is no major loss - the one we have is essentially a poor-mans version - it basically just applies gain to the stream which is then lowered (where necessary) by the volume limiter - poor man's DRC basically.

It would be great to get this in the May window.

@jmarshallnz jmarshallnz referenced this pull request May 6, 2012
Closed

AudioEngine #900

@gnif
Team Kodi member
gnif commented May 9, 2012

I am happy with where ae_rebase is right now, and even with its known issues I feel it is ready for merge to master, so here is my +1 :)

@amejia1
Team Kodi member
amejia1 commented May 9, 2012

The build system changes LGTM and I see now XBMC doesn't blast my computer's volume to max when playing anything using this branch, so +1 from me.

@DDDamian
DDDamian commented May 9, 2012

I've been involved almost daily with this project for several months, working mostly at Win32 and filters development, but also general troubleshooting and arranging test builds and gathering bug reports. For Linux/Win I'm probably the second most familiar with the code.

IMO the code is ready for this merge window. It won't be perfect for months (if ever) as there's much planned for addition and we're at the blue-moon bug level for most testers now. It does what it says on the can - quite well.

+1 for this merge window.

@elupus
Team Kodi member
elupus commented May 9, 2012
@jmarshallnz
Team Kodi member

Thanks all. Will rebase up and hit the big green button :)

@davilla
davilla commented May 9, 2012

fair enough, they don't belong there and it's something that needs to get resolved anyway.

Jonathan Mar... and others added some commits May 9, 2012
Jonathan Marshall fire OnUpdate events when the thumb or fanart is updated in the video…
… info dialog, fixes #13005.
0e6b478
@gnif gnif removes unneeded headers 208605c
@gnif gnif remove unused audiooutput.passthroughmp* eccc2ac
@gnif gnif add SSE method for MathUtils::round_int() 1363a62
@gnif gnif cosmetic: AdvancedSettings::m_musicResample -> m_audioResample 79e46f1
@gnif gnif cosmetic: audioframe.channels -> audioframe.channel_count e5ed6b0
@gnif gnif [AE] settings: Add setting to disable GUI sounds during playback af92212
@gnif gnif [AE] build system support for HAS/HAVE_ALSA e407d10
@gnif gnif [AE] change pulseaudio to default to no rather than auto 532abe2
@gnif gnif [AE] av_crc_init added to libavutil interface 3d6d6cd
@gnif gnif [AE] core: add base files and support for ALSA/OSS 369984f
Jonathan Marshall [AE] volume: drop unneeded g_settings.m_iPreMuteVolumeLevel as settin…
…g volume to zero is no longer used to mute
da81ff3
@DDDamian DDDamian [AE] core: add win32 sinks 74d6a48
@topfs2 topfs2 [AE] core: add PulseAudio engine f540c34
huceke [AE] core: add coreaudio engine 14f17e7
@gnif gnif [AE] update .gitignore b516c35
@gnif gnif [AE] removal of g_audioContext 288be2c
@gnif gnif [AE] add new files to build system bf1b6e3
@gnif gnif [AE] remove SDL_mixer and HAS_SDL_AUDIO e58e6b1
@gnif gnif [AE] visualisations: change AudioData to use floats e529389
Jonathan Marshall [AE] volume: remove dynamic range compression support at the app level 926cf9e
@gnif gnif [AE] volume: Use a float in the range [0,1] for volume cdeef46
@gnif gnif [AE] settings: add new advanced settings f2a704b
@gnif gnif [AE] settings: change 'Output stereo to all speakers' -> 'Enable ster…
…eo upmix'
f35d6d2
@gnif gnif [AE] gui: no need to check passthrough when calling g_audioManager.En…
…able()
299273c
@gnif gnif [AE] settings: Add new settings for AE and reorganise code 5eda08f
@gnif gnif [AE] gui: switch to AE 9311cdb
@gnif gnif [AE] dvdplayer: drop vis callbacks 964a150
Jonathan Marshall [AE] players: change SetVolume to take a float 217694d
@gnif gnif [AE] dvdplayer: switch to use AE 08b78c8
@gnif gnif [AE] paplayer: drop ReadSamples/HasFloatData from codecs 461207b
@gnif gnif [AE] paplayer: add GetChannelInfo to codecs 67638dc
@gnif gnif [AE] paplayer: dvdplayercodec can support passthrough 83b6497
@gnif gnif [AE] paplayer: cosmetic cleanup of MP3Codec a470796
@gnif gnif [AE] paplayer: drop m_BitsPerSampleInternal in MP3Codec 48abb61
@gnif gnif [AE] paplayer: switch MP3Codec to pass through signed 32bpp data 9ea3255
@gnif gnif [AE] paplayer: switch to AE 349ec40
@gnif gnif [AE] remove old files from the build system 99efc86
@DDDamian
DDDamian commented May 9, 2012

Great news.

@jmarshallnz - I would give it one last pr from gnif's branch before you do final rebase - there were three notable fixes that went in during the last 24hr scramble. Gnif will likely be awake again in a few hours. Cheers.

@jmarshallnz jmarshallnz merged commit 19df5c9 into xbmc:master May 9, 2012
@ronie
Team Kodi member

string 407 didn't make it into the .po file and thus shows up as empty in the skin.

Team Kodi member

As I see we have this string duplicated with ids 347 and 407 but at least with this commit we only use 407.
I suggest removing 407 and change line477 to use string 347.

Team Kodi member

Done.

@DDDamian
DDDamian commented May 9, 2012

thx ronie - just got new of that too - it's the DTS-MA string - woohoo the first patch is coming :)

@jmarshallnz
Team Kodi member

@alanwww1 Can you please fix this up AND remove strings.xml from git so that it doesn't happen again.

@DDDamian

@alanwww1 - string 407 should be "- DTS-MA capable receiver"

@rene-ww

Syntax error (space missing) in Main.cpp:143:36: error: expected ')' before ';' token

Team Kodi member

which main (multiple here in this commit)? You can comment the exact line by clicking at the very left of the line...

@rene-ww

I'M getting this error: Main.cpp:143:36: error: expected ')' before ';' token

Team Kodi member

fixed - thx

@gnif

Please fix this to use CAEConvert::Float_S16NE to take advantage of platform instructions (SSE/NEON)

Not possible, viz's are considered addons and as such they should not be assessing xbmc core functions.

@Montellese
Team Kodi member

@gnif This rewrite is missing the calls to m_callback.OnPlaybackPaused() and m_callback.OnPlaybackResumed() which both addons and HTTP-API/JSON-RPC clients (e.g. the official android remote) rely on to keep track of the current playback in XBMC. I haven't tested all the other callbacks but what I also noticed is that m_callback.OnPlaybackStarted() is always called/executed twice. OnPlaybackSpeedChanged and OnPlaybackStopped work fine.

@DDDamian

Good catch spiff - PAPlayer has been exhibiting very high cpu usage

@Montellese
Team Kodi member

Why do we call SetHardwareVolume() directly here instead of going through SetVolume() which contains some extra logic (sending a JSON-RPC notification that the volume has changed)?

@bobo1on1
Team Kodi member

Why?

Team Kodi member
@FernetMenta

@DDDamian @anssih
could you please confirm if this is wrong or correct? shouldn't this be 16 for AC3?

Not just for AC3, but if this is for bitstreaming, then I think all formats (AAC - DTS-HD) should be 16 bits here?

Team Kodi member
Team Kodi member

16 would seem to be more correct, yes. However, I didn't check how exactly is this function used.

Team Kodi member

"more correct" might be the proper term her :) What about TRUEHD? The standard says up to 24bit. Do we need to parse the header instead of assuming a constant?

Team Kodi member

Bitstreaming is always 16bit regardless of the actual payload sample width.

Team Kodi member

Was just going to say that. What can change is samplerate and number of channels. Samplerate need to match (or possibly be the double), but number of channels is only to make room for the larger amount of data.

@FernetMenta

can anybody explain why only 512/2 samples per channel are considered here? Eden just ignored the rest. You can't run a fft when ignoring the rest of the samples, right?

DWORD CDVDAudio::AddPackets(const DVDAudioFrame &audioframe)
{
  CSingleLock lock (m_critSection);

  unsigned char* data = audioframe.data;
  DWORD len = audioframe.size;

  DWORD total = len;
  DWORD copied;

  //Feed audio to the visualizer if necessary.
  if(m_pCallback && !m_bPassthrough)
    m_pCallback->OnAudioData(data, len);

SoftAE does something I don't follow:

  /* we have a frame, if we have a viz we need to hand the data to it */
  if (m_audioCallback && !m_packet->vizData.CursorEnd())
  {
    float *vizData = (float*)m_packet->vizData.CursorRead(2 * sizeof(float));
    memcpy(m_vizBuffer + m_vizBufferSamples, vizData, 2 * sizeof(float));
    m_vizBufferSamples += 2;
    if (m_vizBufferSamples == 512)
    {
      m_audioCallback->OnAudioData(m_vizBuffer, 512);
      m_vizBufferSamples = 0;
    }
  }

Does it take one sample for two channels of a continuous stream and delivers when it has reached a size of 512? The API does not expose a max of 512. If you deliver more, it crashes badly.

Team Kodi member

Eden ran it in 512 blocks and threw away the rest, yes. While it's inaccurate, the result in the vis will be still close enough. IIRC most of the winamp vis's used to do the same thing.

SoftAE is taking every sample by the looks, as I think that block is run on a per-sample basis?

Team Kodi member

thanks for explaining.

as I think that block is run on a per-sample basis?

not very resource-friendly :)

@dalehamel dalehamel pushed a commit to RasPlex/plex-home-theatre that referenced this pull request Jan 9, 2014
@tru tru Fix saving settings to channels
Fixes #939
bd613bd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.