Skip to content

Conversation

@kylegordon
Copy link
Contributor

Use GCC 4.8 in the Makefile rather than editing global system settings.
Use sudo to execute make install

Tested with Ubuntu 15.10, but not Raspbian.

Alan Smillie and others added 3 commits January 26, 2016 17:55
This commit fixes playback on the Windows 8 and 10 platforms.

The behaviour of these platforms differs from the development (Windows 7)
platform in that if the ByteStream supplied to the SourceReader does not
have the MFBYTESTREAM_IS_SEEKABLE capability the stream recognition will
fail without even reading any data.

On Windows 7 the stream recognition phase executed seeks regardless of the
capability setting.

Highlights
----------

1. Enable the MFBYTESTREAM_IS_SEEKABLE capability in the OpenHomeByteStream.
   This allows stream recognition to succeed. It does not enable stream
   seeking from a control point.

2. When OpenHomeByteStream receives a seek request outside of the initial
   recognition cache, during recognition, it updates iStreamPos to the
   requested position and passes the seek.

   The subsequent read request will execute an out of band read of the
   requested size from the 'seeked' location storing it at the start of
   the existing recognition cache.

   The required data will then be returned from the updated cache.

   NB. The out of bound read requires the IWriter interface be implemented.

3. Standardise on TBOOL to avoid passing pointers to TBOOL and casting
   to BOOL. I suspect this was causing some subtle data corruption.

4. In Process() check for stream start/end exception after all
   calls to ReadSample() so that can be passed back to the CodecController
   in a timely manner.

   Also avoid calling FlushPCM() on stream exceptions as it was seen to cause
   crashes.
@DoomHammer
Copy link
Contributor

I don't think changing compiler to g++-4.8 is a good idea. First of all, system that have a different version will not be able to build. Second, you can always override compiler by running make CC=g++-4.8.

Thing worth noticing is that CC is used incorrectly here, since this variable should point to a C compiler, whereas CXX should point to a C++ compiler (G++ or clang++).

As for suggesting sudo I am also not so sure, but this is a matter of taste. There are systems that don't have sudo installed at all and then there is possibility to install ohPlayer to a local prefix that does not require superuser privileges.

@kylegordon
Copy link
Contributor Author

From the sudo perspective, sudo is used in earlier commands so the presumption is that the current user is unprivileged. The make install will then fail unless prefixed with sudo. I guess I'm leaning towards consistency rather than insistence on sudo.

If sudo is undesired, the docs should suggest that the user su - to root first, before apt-get, make install, etc.

@kylegordon
Copy link
Contributor Author

For the Makefile, the issue is that it doesn't work on anything except 4.8. Many systems have different versions available, but not defaulted. Global settings should not be demanded in order to make an exception work. The exception (ohplayer, in this case) should have a requirement on 4.8.

I agree I've probably got CC and CXX incorrect. I'll update tomorrow evening and see how it goes. If need be I'll try and resubmit a patch, but I still feel strongly that the Makefile should be requiring 4.8, rather than messing with system-wide globals

@gordonjcp
Copy link

Yeah, it should either have sudo throughout or not at all.

Regarding gcc-4.8, it would be great if things could be fixed to compile in 4.9/5.0 without breaking back compatibility.

@DoomHammer
Copy link
Contributor

In my opinion burying sudo inside scripts is not a good idea. Users should be aware of what privileges are needed and only use elevated privileges when necessary.

@DoomHammer
Copy link
Contributor

I have some time to work on this next week, so will try to find a good approach, but considering my other two PRs haven't made it through yet I don't know how long it will take. Especially #5 will help to maintain the system.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants