Skip to content

build fixes for Ubuntu 16.04.2#11

Open
cocoon wants to merge 1 commit into
neutrinolabs:develfrom
cocoon:Ubuntu1604
Open

build fixes for Ubuntu 16.04.2#11
cocoon wants to merge 1 commit into
neutrinolabs:develfrom
cocoon:Ubuntu1604

Conversation

@cocoon
Copy link
Copy Markdown
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
Copy Markdown

hogetaro commented Oct 2, 2017

+1
it works for me, thanks

@speidy
Copy link
Copy Markdown
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 suggested changes Apr 5, 2018
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread 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
Copy Markdown
Member

@speidy speidy Apr 5, 2018

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
Copy Markdown
Contributor Author

@cocoon cocoon Apr 5, 2018

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

Comment thread channels/xrdpvr/xrdpvr_player.c
!defined(DISTRO_UBUNTU1111) && !defined(DISTRO_UBUNTU1204) && \
!defined(DISTRO_DEBIAN7) && !defined(DISTRO_UBUNTU1404) && \
!defined(DISTRO_DEBIAN8)
!defined(DISTRO_UBUNTU1604) && !defined(DISTRO_DEBIAN8)
Copy link
Copy Markdown
Member

@speidy speidy Apr 5, 2018

Choose a reason for hiding this comment

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

remove tab after &&

Comment thread libfreerdp-codec/color.c
}

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
Copy Markdown
Member

@speidy speidy Apr 5, 2018

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
Copy Markdown
Contributor Author

@cocoon cocoon Apr 5, 2018

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.

Comment thread channels/drdynvc/tsmf/ffmpeg/tsmf_ffmpeg.c

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

@speidy speidy Apr 5, 2018

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

Comment thread channels/drdynvc/tsmf/ffmpeg/tsmf_ffmpeg.c
mdecoder->codec_context->channels = media_type->Channels;
mdecoder->codec_context->block_align = media_type->BlockAlign;

#if LIBAVCODEC_VERSION_MAJOR < 55
Copy link
Copy Markdown
Member

@speidy speidy Apr 5, 2018

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)

Comment thread channels/drdynvc/tsmf/ffmpeg/tsmf_ffmpeg.c

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
Copy Markdown
Member

@speidy speidy Apr 5, 2018

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

Comment thread channels/xrdpvr/xrdpvr_player.c
AVFrame *audio_frame;
AVFrame *video_frame;
#if defined(DISTRO_DEBIAN8)
#if defined(DISTRO_DEBIAN8) || defined(DISTRO_UBUNTU1604)
Copy link
Copy Markdown
Member

@speidy speidy Apr 5, 2018

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
Copy Markdown
Member

@speidy speidy Apr 5, 2018

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

1 more reviewer

@speidy speidy speidy requested changes

Reviewers whose approvals may not affect merge requirements

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