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.

Sign up for GitHub

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

Jump to bottom

build fixes for Ubuntu 16.04.2 #11

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from
Open

build fixes for Ubuntu 16.04.2 #11

wants to merge 1 commit into from

Conversation

cocoon
Copy link
Contributor

@cocoon cocoon commented Mar 8, 2017 •
edited
Loading

fixes #7

based on PR from github user "eroen": FreeRDP/FreeRDP#1610
changed INLINE -> static INLINE
(this pull request again as single change without revert)

@cocoon cocoon changed the title build fixes for Ubuntu 16.04.2, build fixes for Ubuntu 16.04.2 Mar 8, 2017
@hogetaro
Copy link

hogetaro commented Oct 2, 2017

+1
it works for me, thanks

@speidy
Copy link
Member

speidy commented Apr 5, 2018

broken for me on archlinux with those versions:

➜ ffmpeg --version
ffmpeg version 3.4.2 Copyright (c) 2000-2018 the FFmpeg developers
  built with gcc 7.3.0 (GCC)
  configuration: --prefix=/usr --disable-debug --disable-static --disable-stripping --enable-avisynth --enable-avresample --enable-fontconfig --enable-gmp --enable-gnutls --enable-gpl --enable-ladspa --enable-libass --enable-libbluray --enable-libfreetype --enable-libfribidi --enable-libgsm --enable-libiec61883 --enable-libmodplug --enable-libmp3lame --enable-libopencore_amrnb --enable-libopencore_amrwb --enable-libopenjpeg --enable-libopus --enable-libpulse --enable-libsoxr --enable-libspeex --enable-libssh --enable-libtheora --enable-libv4l2 --enable-libvidstab --enable-libvorbis --enable-libvpx --enable-libwebp --enable-libx264 --enable-libx265 --enable-libxcb --enable-libxml2 --enable-libxvid --enable-shared --enable-version3 --enable-omx
  libavutil      55. 78.100 / 55. 78.100
  libavcodec     57.107.100 / 57.107.100
  libavformat    57. 83.100 / 57. 83.100
  libavdevice    57. 10.100 / 57. 10.100
  libavfilter     6.107.100 /  6.107.100
  libavresample   3.  7.  0 /  3.  7.  0
  libswscale      4.  8.100 /  4.  8.100
  libswresample   2.  9.100 /  2.  9.100
  libpostproc    54.  7.100 / 54.  7.100

speidy
speidy requested changes Apr 5, 2018
View reviewed changes
Copy link
Member

@speidy speidy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@cocoon Thanks for this contribution.

I know FFmpeg developers break the API every once in a while and this task might not be easy.

While it builds on ubuntu 16.04 it think we should re arrange the code better. see my comments.

channels/drdynvc/tsmf/ffmpeg/tsmf_ffmpeg.c
#if LIBAVCODEC_VERSION_MAJOR < 54
#define MAX_AUDIO_FRAME_SIZE AVCODEC_MAX_AUDIO_FRAME_SIZE
#else
#define MAX_AUDIO_FRAME_SIZE 192000
Copy link
Member

Choose a reason for hiding this comment

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

where this sample rate come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is defined in the linked PR, nothing I added.

But it seems to be a generic hard coded limit for whatever reason and represents "1 second of 48khz 32-bit audio"

https://www.ffmpeg.org/doxygen/3.2/libavformat_2dvenc_8c_source.html#l00045
https://lists.ffmpeg.org/pipermail/ffmpeg-cvslog/2012-August/053785.html

channels/xrdpvr/xrdpvr_player.c
@@ -115,7 +124,7 @@ debian 8
#if !defined(DISTRO_DEBIAN6) && !defined(DISTRO_UBUNTU1104) && \
!defined(DISTRO_UBUNTU1111) && !defined(DISTRO_UBUNTU1204) && \
!defined(DISTRO_DEBIAN7) && !defined(DISTRO_UBUNTU1404) && \
!defined(DISTRO_DEBIAN8)
!defined(DISTRO_UBUNTU1604) && !defined(DISTRO_DEBIAN8)
Copy link
Member

Choose a reason for hiding this comment

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

remove tab after &&

libfreerdp-codec/color.c
@@ -86,7 +86,7 @@ void freerdp_set_pixel(uint8* data, int x, int y, int width, int height, int bpp
}
}

INLINE void freerdp_color_split_rgb(uint32* color, int bpp, uint8* red, uint8* green, uint8* blue, uint8* alpha, HCLRCONV clrconv)
static INLINE void freerdp_color_split_rgb(uint32* color, int bpp, uint8* red, uint8* green, uint8* blue, uint8* alpha, HCLRCONV clrconv)
Copy link
Member

Choose a reason for hiding this comment

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

seems like those changes are irrelevant to this PR (not an ffmpeg api fixes)
either split it to another PR or remove them for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes makes sense.
Will try to make an own PR for these changes.

channels/drdynvc/tsmf/ffmpeg/tsmf_ffmpeg.c
typedef struct _TSMFFFmpegDecoder
{
ITSMFDecoder iface;

int media_type;
enum CodecID codec_id;
#if LIBAVCODEC_VERSION_MAJOR < 55
Copy link
Member

Choose a reason for hiding this comment

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

I prefer not to pollute the code with inline ifdefs
we should use the latest API and backport it using our own defines (at the head of this file or smthg).

For example

#if LIBAVCODEC_VERSION_MAJOR < 55
#define AVCodecID CodecID
#endif

channels/drdynvc/tsmf/ffmpeg/tsmf_ffmpeg.c
@@ -89,6 +99,7 @@ static tbool tsmf_ffmpeg_init_audio_stream(ITSMFDecoder* decoder, const TS_AM_ME
mdecoder->codec_context->channels = media_type->Channels;
mdecoder->codec_context->block_align = media_type->BlockAlign;

#if LIBAVCODEC_VERSION_MAJOR < 55
Copy link
Member

Choose a reason for hiding this comment

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

same applies here.
lets make a wrapper for av_set_cpu_flags_mask and backport its code for older api.
(maybe even copy their implementation)

channels/drdynvc/tsmf/ffmpeg/tsmf_ffmpeg.c
@@ -387,7 +406,7 @@ static tbool tsmf_ffmpeg_decode_audio(ITSMFDecoder* decoder, const uint8* data,
#endif

if (mdecoder->decoded_size_max == 0)
mdecoder->decoded_size_max = AVCODEC_MAX_AUDIO_FRAME_SIZE + 16;
mdecoder->decoded_size_max = MAX_AUDIO_FRAME_SIZE + 16;
Copy link
Member

Choose a reason for hiding this comment

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

MAX_AUDIO_FRAME_SIZE shouldnt be defined by us
we should find another way to backport this code

channels/xrdpvr/xrdpvr_player.c
@@ -141,7 +150,7 @@ typedef struct player_state_info
AVCodec *video_codec;
AVFrame *audio_frame;
AVFrame *video_frame;
#if defined(DISTRO_DEBIAN8)
#if defined(DISTRO_DEBIAN8) || defined(DISTRO_UBUNTU1604)
Copy link
Member

Choose a reason for hiding this comment

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

I know its already there but I don't like those DISTRO_xxx backport style. its very specific.
we should backport the code so it builds everywhere with this ffmpeg libs

there's not really an dependency/change on a specific distro

Copy link
Member

Choose a reason for hiding this comment

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

you can keep it like this for now. meh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@speidy speidy speidy requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can't build NeutrinoRDP under Ubuntu 16.04
3 participants
@cocoon @hogetaro @speidy