Skip to content

Commit

Permalink
[AudioEngine] Introduce AEDefines.h with an override for android
Browse files Browse the repository at this point in the history
  • Loading branch information
fetzerch committed Jan 7, 2017
1 parent 3ec336d commit 2a13c92
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 5 deletions.
26 changes: 26 additions & 0 deletions xbmc/cores/AudioEngine/AEDefines.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#pragma once
/*
* Copyright (C) 2010-2017 Team XBMC
* http://xbmc.org
*
* This Program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2, or (at your option)
* any later version.
*
* This Program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with XBMC; see the file COPYING. If not, see
* <http://www.gnu.org/licenses/>.
*
*/

#define AE_AC3_ENCODE_BITRATE 640000
#define AE_DTS_ENCODE_BITRATE 1411200

// Enable platform specific overrides
#include "AEDefines_override.h"
1 change: 1 addition & 0 deletions xbmc/cores/AudioEngine/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ if(CORE_SYSTEM_NAME STREQUAL freebsd)
endif()

core_add_library(audioengine)
add_platform_override(${CORE_LIBRARY} AEDefines.h PLATFORMS android)
target_include_directories(${CORE_LIBRARY} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})
if(NOT CORE_SYSTEM_NAME STREQUAL windows)
if(HAVE_SSE)
Expand Down
7 changes: 2 additions & 5 deletions xbmc/cores/AudioEngine/Encoders/AEEncoderFFmpeg.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@
*
*/

#define AC3_ENCODE_BITRATE 640000
#define DTS_ENCODE_BITRATE 1411200

#include "cores/AudioEngine/AEDefines.h"
#include "cores/AudioEngine/Encoders/AEEncoderFFmpeg.h"
#include "cores/AudioEngine/Utils/AEUtil.h"
#include "utils/log.h"
Expand Down Expand Up @@ -107,7 +105,7 @@ bool CAEEncoderFFmpeg::Initialize(AEAudioFormat &format, bool allow_planar_input
{
m_CodecName = "AC3";
m_CodecID = AV_CODEC_ID_AC3;
m_BitRate = AC3_ENCODE_BITRATE;
m_BitRate = AE_AC3_ENCODE_BITRATE;
codec = avcodec_find_encoder(m_CodecID);
}

Expand Down Expand Up @@ -325,4 +323,3 @@ double CAEEncoderFFmpeg::GetDelay(unsigned int bufferSize)

return ((double)frames + ((double)bufferSize * m_OutputRatio)) * m_SampleRateMul;
}

24 changes: 24 additions & 0 deletions xbmc/cores/AudioEngine/overrides/android/AEDefines.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#pragma once
/*
* Copyright (C) 2010-2017 Team XBMC
* http://xbmc.org
*
* This Program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2, or (at your option)
* any later version.
*
* This Program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with XBMC; see the file COPYING. If not, see
* <http://www.gnu.org/licenses/>.
*
*/

// Several Android TV devices only support 384 kbit/s as maximum
#undef AE_AC3_ENCODE_BITRATE
#define AE_AC3_ENCODE_BITRATE 384000

24 comments on commit 2a13c92

@mkreisl
Copy link

@mkreisl mkreisl commented on 2a13c92 Jan 8, 2017

Choose a reason for hiding this comment

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

#include "AEDefines_override.h"

This file is nowhere (is not generated using autotools build system)

@fritsch
Copy link
Member

@fritsch fritsch commented on 2a13c92 Jan 8, 2017

Choose a reason for hiding this comment

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

Autotools is deprecated and already dropped in master. But yes, that will break autotools on Krypton. @fetzerch do you have an idea of fix up?

@fetzerch
Copy link
Member Author

Choose a reason for hiding this comment

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

Sry, completely forgot about autotools. Don't think it makes any sense to implement this for autotools where we already know that even the cmake part needs a rework. I know that this is not gonna be very popular but: revert and just #ifdef for krypton?

@fritsch
Copy link
Member

@fritsch fritsch commented on 2a13c92 Jan 8, 2017

Choose a reason for hiding this comment

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

@FernetMenta your take on that?

@FernetMenta
Copy link
Contributor

Choose a reason for hiding this comment

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

krypton can be built with cmake on all platforms, right?

@fritsch
Copy link
Member

@fritsch fritsch commented on 2a13c92 Jan 8, 2017

Choose a reason for hiding this comment

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

We still ship autotools and did not deprecate it properly. I don't think it is a good idea to sacrifice that at this point of the release?

@fetzerch
Copy link
Member Author

Choose a reason for hiding this comment

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

It can but there was a decision to remove autotools only for v18 to give distro maintainers enough time to switch. forcing them now just for this feels kinda rushed.

@mkreisl
Copy link

@mkreisl mkreisl commented on 2a13c92 Jan 8, 2017

Choose a reason for hiding this comment

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

That's right. We are on RC and breaking autotools in this state is ridiculous

@FernetMenta
Copy link
Contributor

@FernetMenta FernetMenta commented on 2a13c92 Jan 8, 2017

Choose a reason for hiding this comment

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

I won't agree to the ifdef. This PR is almost 2 months old and inactivity does not justify hacks of a result of urgency close to release. Apart from this it is no regression.

@fetzerch
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, but it's also not feasible to implement something generic that is used exactly in 1 place for a buildsystem we have already dropped.

@fritsch
Copy link
Member

@fritsch fritsch commented on 2a13c92 Jan 8, 2017 via email

Choose a reason for hiding this comment

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

@fritsch
Copy link
Member

@fritsch fritsch commented on 2a13c92 Jan 8, 2017

Choose a reason for hiding this comment

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

Will PR something in three minutes ...

@fritsch
Copy link
Member

@fritsch fritsch commented on 2a13c92 Jan 8, 2017

Choose a reason for hiding this comment

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

@mkreisl would that work: #11399 - no I am not really proud about that, but I need this bitrate overwrite on android. As without having 384 kbit ac3 transcoder support, we would ruin multichannel on many thousand TV devices. This was not a problem in v16, as we did not use the official API of Audiotrack back then and the 640 worked, as we bypassed the broken firmware.

@mkreisl
Copy link

@mkreisl mkreisl commented on 2a13c92 Jan 8, 2017

Choose a reason for hiding this comment

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

@fritsch
Looks ok 😄

@fritsch
Copy link
Member

@fritsch fritsch commented on 2a13c92 Jan 8, 2017

Choose a reason for hiding this comment

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

@mkreisl as autotools specialist, would that also work for you: #11400 ?

@mkreisl
Copy link

@mkreisl mkreisl commented on 2a13c92 Jan 8, 2017

Choose a reason for hiding this comment

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

@fritsch
Much better, tested and works 👍
That what I did manually

@fritsch
Copy link
Member

@fritsch fritsch commented on 2a13c92 Jan 8, 2017 via email

Choose a reason for hiding this comment

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

@mkreisl
Copy link

@mkreisl mkreisl commented on 2a13c92 Jan 8, 2017

Choose a reason for hiding this comment

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

I'll wait a bit before switching to cmake / v18 until most issues have been fixed using cmake build system

@fritsch
Copy link
Member

@fritsch fritsch commented on 2a13c92 Jan 8, 2017 via email

Choose a reason for hiding this comment

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

@mkreisl
Copy link

@mkreisl mkreisl commented on 2a13c92 Jan 8, 2017

Choose a reason for hiding this comment

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

No, but there are still a lot of cmake build commits 😃

@Memphiz
Copy link
Member

Choose a reason for hiding this comment

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

no he is not the last autotools user ... my tvos branch is using it too and this change breaks it.

@fritsch
Copy link
Member

@fritsch fritsch commented on 2a13c92 Jan 15, 2017 via email

Choose a reason for hiding this comment

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

@Memphiz
Copy link
Member

Choose a reason for hiding this comment

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

@fritsch - no its not ... because you guys ignore xcode all the time - i know its unintentional - but its a fact ;)

@fritsch
Copy link
Member

@fritsch fritsch commented on 2a13c92 Jan 15, 2017 via email

Choose a reason for hiding this comment

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

Please sign in to comment.