Media Hub

Merge lp://qastaging/~justinmcp/media-hub/proxy-player into lp://qastaging/media-hub

  1. proxy-player
  2. Merge into trunk
Proposed by Justin McPherson
Status: Approved
Approved by: Jim Hodapp
Approved revision: 165
Proposed branch: lp://qastaging/~justinmcp/media-hub/proxy-player
Merge into: lp://qastaging/media-hub
Diff against target: 917 lines (+625/-17)
19 files modified
CMakeLists.txt (+1/-1)
debian/VERSION (+1/-1)
debian/VERSION.vivid (+1/-1)
debian/changelog (+6/-0)
include/core/media/player.h (+8/-0)
src/core/media/CMakeLists.txt (+1/-0)
src/core/media/codec.h (+40/-0)
src/core/media/player.cpp (+13/-0)
src/core/media/player_configuration.h (+2/-0)
src/core/media/player_implementation.cpp (+6/-0)
src/core/media/player_implementation.h (+1/-0)
src/core/media/player_stub.cpp (+12/-3)
src/core/media/player_stub.h (+3/-1)
src/core/media/proxy_player_implementation.cpp (+404/-0)
src/core/media/proxy_player_implementation.h (+90/-0)
src/core/media/service_implementation.cpp (+21/-6)
src/core/media/service_implementation.h (+3/-0)
src/core/media/service_skeleton.cpp (+5/-0)
src/core/media/service_stub.cpp (+7/-4)
To merge this branch: bzr merge lp://qastaging/~justinmcp/media-hub/proxy-player
Reviewer Review Type Date Requested Status
Jim Hodapp (community) code Approve
PS Jenkins bot continuous-integration Needs Fixing
Santosh Pending
Review via email: mp+268862@code.qastaging.launchpad.net

Commit message

Support for external media players managed by media-hub.

To post a comment you must log in.
Revision history for this message
Jim Hodapp (jhodapp) wrote :

See suggested changes inline

review: Needs Fixing (code)
Reply
Revision history for this message
Justin McPherson (justinmcp) :
Reply
Revision history for this message
Jim Hodapp (jhodapp) :
Reply
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:155
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~justinmcp/media-hub/proxy-player/+merge/268862/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/media-hub-ci/377/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/217/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/217/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/217/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/377/rebuild

review: Needs Fixing (continuous-integration)
Reply
lp://qastaging/~justinmcp/media-hub/proxy-player updated
156. By Justin McPherson

Merge from trunk.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:156
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~justinmcp/media-hub/proxy-player/+merge/268862/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/media-hub-ci/404/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-amd64-ci/244
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/244
        deb: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-armhf-ci/244/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/media-hub-vivid-i386-ci/244

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/media-hub-ci/404/rebuild

review: Needs Fixing (continuous-integration)
Reply
Revision history for this message
Jim Hodapp (jhodapp) :
Reply
Revision history for this message
Justin McPherson (justinmcp) :
Reply
Revision history for this message
Jim Hodapp (jhodapp) :
Reply
Revision history for this message
Justin McPherson (justinmcp) :
Reply
Revision history for this message
Jim Hodapp (jhodapp) :
Reply
Revision history for this message
Jim Hodapp (jhodapp) wrote :

Several comments inline below after doing a more thorough code review. Also, this MR needs syncing with the latest media-hub branch which includes the use of a new logging facility instead of using cout/cerr.

review: Needs Fixing (code)
Reply
lp://qastaging/~justinmcp/media-hub/proxy-player updated
157. By Justin McPherson

Enable variant session type through the Player Configuration structure.

Revision history for this message
Jim Hodapp (jhodapp) wrote :

This is definitely what I had in mind and looks great. Thanks for doing it. A couple of minor comments inline below and then you just need to double check the client_death_cb related handling. Also, make sure to rebase this branch from trunk and convert to using the Logger instead of cout/cerr.

Reply
Revision history for this message
Justin McPherson (justinmcp) :
Reply
Revision history for this message
Justin McPherson (justinmcp) :
Reply
Revision history for this message
Jim Hodapp (jhodapp) :
Reply
Revision history for this message
Justin McPherson (justinmcp) :
Reply
lp://qastaging/~justinmcp/media-hub/proxy-player updated
158. By Justin McPherson

Remove unneeded dbus method definition

159. By Justin McPherson

Merge from trunk.

160. By Justin McPherson

- Use new logging functionality
- Fix en/decoding of PlayerType
- Return a default player type if type arg is unknown

Revision history for this message
Jim Hodapp (jhodapp) :
Reply
Revision history for this message
Jim Hodapp (jhodapp) wrote :

Also I was curious how you've been testing this out? Does it already link to Oxide in some way?

Reply
Revision history for this message
Justin McPherson (justinmcp) wrote :

Sometime ago, this branch was working with https://code.launchpad.net/~justinmcp/oxide/media-arbitration.

Reply
lp://qastaging/~justinmcp/media-hub/proxy-player updated
161. By Justin McPherson

Rename PlayerType enum values.

rigged -> standard
proxy -> proxied

162. By Justin McPherson

Renable test

163. By Justin McPherson

Changelog entry

Revision history for this message
Jim Hodapp (jhodapp) wrote :

You don't need an explicit changelog entry here. Going through the silo process will take the commit message from LP and add a changelog entry.

review: Needs Fixing
Reply
lp://qastaging/~justinmcp/media-hub/proxy-player updated
164. By Justin McPherson

Remove entry

Revision history for this message
Robert Bruce Park (robru) wrote :

Please don't delete debian/changelog. Just revert all your changes to debian/changelog and then everything should work fine.

Reply
lp://qastaging/~justinmcp/media-hub/proxy-player updated
165. By Justin McPherson

Fix changelog

Revision history for this message
Justin McPherson (justinmcp) wrote :

Before my morning coffee, apologies, the latter was meant, the former was done. Fixed...

Reply
Revision history for this message
Jim Hodapp (jhodapp) wrote :

LGTM

review: Approve (code)
Reply
lp://qastaging/~justinmcp/media-hub/proxy-player updated
166. By Justin McPherson

Support for older compilers.

167. By Justin McPherson

Bump version number

168. By Justin McPherson

Update version is VERSION* files

169. By Justin McPherson

Merge from trunk.

170. By Justin McPherson

Fix build

171. By Justin McPherson

Merge from trunk.

172. By Justin McPherson

Merge from trunk.

Unmerged revisions

172. By Justin McPherson

Merge from trunk.

171. By Justin McPherson

Merge from trunk.

170. By Justin McPherson

Fix build

169. By Justin McPherson

Merge from trunk.

168. By Justin McPherson

Update version is VERSION* files

167. By Justin McPherson

Bump version number

166. By Justin McPherson

Support for older compilers.

165. By Justin McPherson

Fix changelog

164. By Justin McPherson

Remove entry

163. By Justin McPherson

Changelog entry

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches