Merge lp:~alfonsosanchezbeato/media-hub/get-selected-streams into lp:media-hub
- get-selected-streams
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 156 |
Proposed branch: | lp:~alfonsosanchezbeato/media-hub/get-selected-streams |
Merge into: | lp:media-hub |
Diff against target: |
291 lines (+119/-11) (has conflicts) 6 files modified
debian/changelog (+15/-4) debian/control (+1/-0) src/core/media/CMakeLists.txt (+2/-0) src/core/media/gstreamer/engine.cpp (+22/-5) src/core/media/gstreamer/playbin.cpp (+72/-2) src/core/media/gstreamer/playbin.h (+7/-0) Text conflict in debian/changelog |
To merge this branch: | bzr merge lp:~alfonsosanchezbeato/media-hub/get-selected-streams |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jim Hodapp (community) | code | Approve | |
Review via email: mp+266251@code.launchpad.net |
Commit message
Gather information about missing codecs and selected streams from gstreamer
so we have more information on whether we should play the video or not (LP: #1417950).
Description of the change
Gather information about missing codecs and selected streams from gstreamer
so we have more information on whether we should play the video or not (LP: #1417950).
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote : | # |
Comments addressed. @Jim, yes, for the moment this is it until we modify media-hub's DBus interface so we can transmit more information to the client like:
* Available tracks with type and description
* Information on which tracks can be played and which not due to missing codecs
* A way to select the tracks to play
* An intermediate state in which we can access to this information but we are still not playing (gstreamer state for the stream would be "paused"). Maybe we can move to that state when media is set instead of moving from the initial gstreamer state to playing when the DBus play() method is invoked.
Jim Hodapp (jhodapp) wrote : | # |
See my comments below.
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote : | # |
Comments addressed
Jim Hodapp (jhodapp) wrote : | # |
Looks great, thanks. Let me know when you have this in a silo and I'll help test this well.
- 149. By Alfonso Sanchez-Beato
-
Gather information about missing codecs and selected streams from gstreamer
so we have more information on whether we should play the video or not.
Preview Diff
1 | === modified file 'debian/changelog' | |||
2 | --- debian/changelog 2015-07-24 14:54:21 +0000 | |||
3 | +++ debian/changelog 2015-08-10 13:09:04 +0000 | |||
4 | @@ -1,3 +1,4 @@ | |||
5 | 1 | <<<<<<< TREE | ||
6 | 1 | media-hub (3.1.0+15.10.20150724-0ubuntu1) wily; urgency=medium | 2 | media-hub (3.1.0+15.10.20150724-0ubuntu1) wily; urgency=medium |
7 | 2 | 3 | ||
8 | 3 | [ Alfonso Sanchez-Beato (email Canonical) ] | 4 | [ Alfonso Sanchez-Beato (email Canonical) ] |
9 | @@ -7,6 +8,16 @@ | |||
10 | 7 | -- CI Train Bot <ci-train-bot@canonical.com> Fri, 24 Jul 2015 14:54:21 +0000 | 8 | -- CI Train Bot <ci-train-bot@canonical.com> Fri, 24 Jul 2015 14:54:21 +0000 |
11 | 8 | 9 | ||
12 | 9 | media-hub (3.1.0+15.10.20150710-0ubuntu1) wily; urgency=medium | 10 | media-hub (3.1.0+15.10.20150710-0ubuntu1) wily; urgency=medium |
13 | 11 | ======= | ||
14 | 12 | media-hub (3.1.0+15.04.20150810-0ubuntu1) UNRELEASED; urgency=medium | ||
15 | 13 | |||
16 | 14 | * Gather information about missing codecs and selected streams from gstreamer | ||
17 | 15 | so we have more information on whether we should play the video or not. | ||
18 | 16 | |||
19 | 17 | -- Alfonso Sanchez-Beato (email Canonical) <alfonso.sanchez-beato@canonical.com> Mon, 10 Aug 2015 15:04:18 +0200 | ||
20 | 18 | |||
21 | 19 | media-hub (3.1.0+15.04.20150710-0ubuntu1) vivid; urgency=medium | ||
22 | 20 | >>>>>>> MERGE-SOURCE | ||
23 | 10 | 21 | ||
24 | 11 | [ CI Train Bot ] | 22 | [ CI Train Bot ] |
25 | 12 | * New rebuild forced. | 23 | * New rebuild forced. |
26 | @@ -17,7 +28,7 @@ | |||
27 | 17 | 28 | ||
28 | 18 | -- CI Train Bot <ci-train-bot@canonical.com> Fri, 10 Jul 2015 18:12:21 +0000 | 29 | -- CI Train Bot <ci-train-bot@canonical.com> Fri, 10 Jul 2015 18:12:21 +0000 |
29 | 19 | 30 | ||
31 | 20 | media-hub (3.1.0+15.10.20150604-0ubuntu1) wily; urgency=medium | 31 | media-hub (3.1.0+15.04.20150604-0ubuntu1) vivid; urgency=medium |
32 | 21 | 32 | ||
33 | 22 | [ CI Train Bot ] | 33 | [ CI Train Bot ] |
34 | 23 | * New rebuild forced. | 34 | * New rebuild forced. |
35 | @@ -28,7 +39,7 @@ | |||
36 | 28 | 39 | ||
37 | 29 | -- CI Train Bot <ci-train-bot@canonical.com> Thu, 04 Jun 2015 14:18:22 +0000 | 40 | -- CI Train Bot <ci-train-bot@canonical.com> Thu, 04 Jun 2015 14:18:22 +0000 |
38 | 30 | 41 | ||
40 | 31 | media-hub (3.1.0+15.10.20150601-0ubuntu1) wily; urgency=medium | 42 | media-hub (3.1.0+15.04.20150601-0ubuntu1) vivid; urgency=medium |
41 | 32 | 43 | ||
42 | 33 | [ CI Train Bot ] | 44 | [ CI Train Bot ] |
43 | 34 | * New rebuild forced. | 45 | * New rebuild forced. |
44 | @@ -39,7 +50,7 @@ | |||
45 | 39 | 50 | ||
46 | 40 | -- CI Train Bot <ci-train-bot@canonical.com> Mon, 01 Jun 2015 16:31:53 +0000 | 51 | -- CI Train Bot <ci-train-bot@canonical.com> Mon, 01 Jun 2015 16:31:53 +0000 |
47 | 41 | 52 | ||
49 | 42 | media-hub (3.1.0+15.10.20150527.1-0ubuntu1) wily; urgency=medium | 53 | media-hub (3.1.0+15.04.20150527.1-0ubuntu1) vivid; urgency=medium |
50 | 43 | 54 | ||
51 | 44 | [ CI Train Bot ] | 55 | [ CI Train Bot ] |
52 | 45 | * New rebuild forced. | 56 | * New rebuild forced. |
53 | @@ -50,7 +61,7 @@ | |||
54 | 50 | 61 | ||
55 | 51 | -- CI Train Bot <ci-train-bot@canonical.com> Wed, 27 May 2015 18:38:16 +0000 | 62 | -- CI Train Bot <ci-train-bot@canonical.com> Wed, 27 May 2015 18:38:16 +0000 |
56 | 52 | 63 | ||
58 | 53 | media-hub (3.1.0+15.10.20150522-0ubuntu1) wily; urgency=medium | 64 | media-hub (3.1.0+15.04.20150522-0ubuntu1) vivid; urgency=medium |
59 | 54 | 65 | ||
60 | 55 | [ Jim Hodapp ] | 66 | [ Jim Hodapp ] |
61 | 56 | * Fix issues with not reporting failed decoding error to the client. | 67 | * Fix issues with not reporting failed decoding error to the client. |
62 | 57 | 68 | ||
63 | === modified file 'debian/control' | |||
64 | --- debian/control 2015-03-12 11:38:32 +0000 | |||
65 | +++ debian/control 2015-08-10 13:09:04 +0000 | |||
66 | @@ -25,6 +25,7 @@ | |||
67 | 25 | libprocess-cpp-dev, | 25 | libprocess-cpp-dev, |
68 | 26 | libproperties-cpp-dev, | 26 | libproperties-cpp-dev, |
69 | 27 | libgstreamer1.0-dev, | 27 | libgstreamer1.0-dev, |
70 | 28 | libgstreamer-plugins-base1.0-dev, | ||
71 | 28 | pkg-config, | 29 | pkg-config, |
72 | 29 | libpulse-dev, | 30 | libpulse-dev, |
73 | 30 | qtbase5-dev, | 31 | qtbase5-dev, |
74 | 31 | 32 | ||
75 | === modified file 'src/core/media/CMakeLists.txt' | |||
76 | --- src/core/media/CMakeLists.txt 2015-02-24 15:22:19 +0000 | |||
77 | +++ src/core/media/CMakeLists.txt 2015-08-10 13:09:04 +0000 | |||
78 | @@ -1,4 +1,5 @@ | |||
79 | 1 | pkg_check_modules(PC_GSTREAMER_1_0 REQUIRED gstreamer-1.0) | 1 | pkg_check_modules(PC_GSTREAMER_1_0 REQUIRED gstreamer-1.0) |
80 | 2 | pkg_check_modules(PC_GSTREAMER_PBUTILS_1_0 REQUIRED gstreamer-pbutils-1.0) | ||
81 | 2 | pkg_check_modules(PC_PULSE_AUDIO REQUIRED libpulse) | 3 | pkg_check_modules(PC_PULSE_AUDIO REQUIRED libpulse) |
82 | 3 | include_directories(${PC_GSTREAMER_1_0_INCLUDE_DIRS} ${HYBRIS_MEDIA_CFLAGS} ${PC_PULSE_AUDIO_INCLUDE_DIRS}) | 4 | include_directories(${PC_GSTREAMER_1_0_INCLUDE_DIRS} ${HYBRIS_MEDIA_CFLAGS} ${PC_PULSE_AUDIO_INCLUDE_DIRS}) |
83 | 4 | 5 | ||
84 | @@ -126,6 +127,7 @@ | |||
85 | 126 | ${DBUS_CPP_LDFLAGS} | 127 | ${DBUS_CPP_LDFLAGS} |
86 | 127 | ${GLog_LIBRARY} | 128 | ${GLog_LIBRARY} |
87 | 128 | ${PC_GSTREAMER_1_0_LIBRARIES} | 129 | ${PC_GSTREAMER_1_0_LIBRARIES} |
88 | 130 | ${PC_GSTREAMER_PBUTILS_1_0_LIBRARIES} | ||
89 | 129 | ${PROCESS_CPP_LDFLAGS} | 131 | ${PROCESS_CPP_LDFLAGS} |
90 | 130 | ${GIO_LIBRARIES} | 132 | ${GIO_LIBRARIES} |
91 | 131 | ${HYBRIS_MEDIA_LIBRARIES} | 133 | ${HYBRIS_MEDIA_LIBRARIES} |
92 | 132 | 134 | ||
93 | === modified file 'src/core/media/gstreamer/engine.cpp' | |||
94 | --- src/core/media/gstreamer/engine.cpp 2015-06-01 16:30:59 +0000 | |||
95 | +++ src/core/media/gstreamer/engine.cpp 2015-08-10 13:09:04 +0000 | |||
96 | @@ -67,8 +67,22 @@ | |||
97 | 67 | { | 67 | { |
98 | 68 | if (p.second == "playbin") | 68 | if (p.second == "playbin") |
99 | 69 | { | 69 | { |
102 | 70 | std::cout << "State changed on playbin: " << p.first.new_state << std::endl; | 70 | std::cout << "State changed on playbin: " |
103 | 71 | playback_status_changed(gst_state_to_player_status(p.first)); | 71 | << gst_element_state_get_name(p.first.new_state) << std::endl; |
104 | 72 | const auto status = gst_state_to_player_status(p.first); | ||
105 | 73 | /* | ||
106 | 74 | * When state moves to "paused" the pipeline is already set. We check that we | ||
107 | 75 | * have streams to play. | ||
108 | 76 | */ | ||
109 | 77 | if (status == media::Player::PlaybackStatus::paused && | ||
110 | 78 | !playbin.can_play_streams()) { | ||
111 | 79 | std::cerr << "** Cannot play: some codecs are missing" << std::endl; | ||
112 | 80 | playbin.reset(); | ||
113 | 81 | const media::Player::Error e = media::Player::Error::format_error; | ||
114 | 82 | error(e); | ||
115 | 83 | } else { | ||
116 | 84 | playback_status_changed(status); | ||
117 | 85 | } | ||
118 | 72 | } | 86 | } |
119 | 73 | } | 87 | } |
120 | 74 | 88 | ||
121 | @@ -149,7 +163,8 @@ | |||
122 | 149 | break; | 163 | break; |
123 | 150 | case GST_STREAM_ERROR_CODEC_NOT_FOUND: | 164 | case GST_STREAM_ERROR_CODEC_NOT_FOUND: |
124 | 151 | std::cerr << "** Encountered a GST_STREAM_ERROR_CODEC_NOT_FOUND" << std::endl; | 165 | std::cerr << "** Encountered a GST_STREAM_ERROR_CODEC_NOT_FOUND" << std::endl; |
126 | 152 | ret_error = media::Player::Error::format_error; | 166 | // Missing codecs are handled later, when state switches to "paused" |
127 | 167 | ret_error = media::Player::Error::no_error; | ||
128 | 153 | break; | 168 | break; |
129 | 154 | case GST_STREAM_ERROR_DECODE: | 169 | case GST_STREAM_ERROR_DECODE: |
130 | 155 | std::cerr << "** Encountered a GST_STREAM_ERROR_DECODE" << std::endl; | 170 | std::cerr << "** Encountered a GST_STREAM_ERROR_DECODE" << std::endl; |
131 | @@ -163,8 +178,10 @@ | |||
132 | 163 | } | 178 | } |
133 | 164 | } | 179 | } |
134 | 165 | 180 | ||
137 | 166 | std::cout << "Resetting playbin pipeline after unrecoverable error" << std::endl; | 181 | if (ret_error != media::Player::Error::no_error) { |
138 | 167 | playbin.reset(); | 182 | std::cerr << "Resetting playbin pipeline after unrecoverable error" << std::endl; |
139 | 183 | playbin.reset(); | ||
140 | 184 | } | ||
141 | 168 | return ret_error; | 185 | return ret_error; |
142 | 169 | } | 186 | } |
143 | 170 | 187 | ||
144 | 171 | 188 | ||
145 | === modified file 'src/core/media/gstreamer/playbin.cpp' | |||
146 | --- src/core/media/gstreamer/playbin.cpp 2015-07-24 14:54:19 +0000 | |||
147 | +++ src/core/media/gstreamer/playbin.cpp 2015-08-10 13:09:04 +0000 | |||
148 | @@ -1,5 +1,5 @@ | |||
149 | 1 | /* | 1 | /* |
151 | 2 | * Copyright © 2013 Canonical Ltd. | 2 | * Copyright © 2013-2015 Canonical Ltd. |
152 | 3 | * | 3 | * |
153 | 4 | * This program is free software: you can redistribute it and/or modify it | 4 | * This program is free software: you can redistribute it and/or modify it |
154 | 5 | * under the terms of the GNU Lesser General Public License version 3, | 5 | * under the terms of the GNU Lesser General Public License version 3, |
155 | @@ -20,6 +20,8 @@ | |||
156 | 20 | 20 | ||
157 | 21 | #include <core/media/gstreamer/engine.h> | 21 | #include <core/media/gstreamer/engine.h> |
158 | 22 | 22 | ||
159 | 23 | #include <gst/pbutils/missing-plugins.h> | ||
160 | 24 | |||
161 | 23 | #if defined(MEDIA_HUB_HAVE_HYBRIS_MEDIA_COMPAT_LAYER) | 25 | #if defined(MEDIA_HUB_HAVE_HYBRIS_MEDIA_COMPAT_LAYER) |
162 | 24 | #include <hybris/media/surface_texture_client_hybris.h> | 26 | #include <hybris/media/surface_texture_client_hybris.h> |
163 | 25 | #include <hybris/media/media_codec_layer.h> | 27 | #include <hybris/media/media_codec_layer.h> |
164 | @@ -112,7 +114,11 @@ | |||
165 | 112 | core::ubuntu::media::video::Width{0}}, | 114 | core::ubuntu::media::video::Width{0}}, |
166 | 113 | player_lifetime(media::Player::Lifetime::normal), | 115 | player_lifetime(media::Player::Lifetime::normal), |
167 | 114 | about_to_finish_handler_id(0), | 116 | about_to_finish_handler_id(0), |
169 | 115 | source_setup_handler_id(0) | 117 | source_setup_handler_id(0), |
170 | 118 | is_missing_audio_codec(false), | ||
171 | 119 | is_missing_video_codec(false), | ||
172 | 120 | audio_stream_id(-1), | ||
173 | 121 | video_stream_id(-1) | ||
174 | 116 | { | 122 | { |
175 | 117 | if (!pipeline) | 123 | if (!pipeline) |
176 | 118 | throw std::runtime_error("Could not create pipeline for playbin."); | 124 | throw std::runtime_error("Could not create pipeline for playbin."); |
177 | @@ -177,6 +183,44 @@ | |||
178 | 177 | std::cout << "Failed to reset the pipeline state. Client reconnect may not function properly." << std::endl; | 183 | std::cout << "Failed to reset the pipeline state. Client reconnect may not function properly." << std::endl; |
179 | 178 | } | 184 | } |
180 | 179 | file_type = MEDIA_FILE_TYPE_NONE; | 185 | file_type = MEDIA_FILE_TYPE_NONE; |
181 | 186 | is_missing_audio_codec = false; | ||
182 | 187 | is_missing_video_codec = false; | ||
183 | 188 | audio_stream_id = -1; | ||
184 | 189 | video_stream_id = -1; | ||
185 | 190 | } | ||
186 | 191 | |||
187 | 192 | void gstreamer::Playbin::process_message_element(GstMessage *message) | ||
188 | 193 | { | ||
189 | 194 | if (!gst_is_missing_plugin_message(message)) | ||
190 | 195 | return; | ||
191 | 196 | |||
192 | 197 | gchar *desc = gst_missing_plugin_message_get_description(message); | ||
193 | 198 | std::cerr << "Missing plugin: " << desc << std::endl; | ||
194 | 199 | g_free(desc); | ||
195 | 200 | |||
196 | 201 | const GstStructure *msg_data = gst_message_get_structure(message); | ||
197 | 202 | if (g_strcmp0("decoder", gst_structure_get_string(msg_data, "type")) != 0) | ||
198 | 203 | return; | ||
199 | 204 | |||
200 | 205 | GstCaps *caps; | ||
201 | 206 | if (!gst_structure_get(msg_data, "detail", GST_TYPE_CAPS, &caps, NULL)) { | ||
202 | 207 | std::cerr << __PRETTY_FUNCTION__ << ": No detail" << std::endl; | ||
203 | 208 | return; | ||
204 | 209 | } | ||
205 | 210 | |||
206 | 211 | GstStructure *caps_data = gst_caps_get_structure(caps, 0); | ||
207 | 212 | if (!caps_data) { | ||
208 | 213 | std::cerr << __PRETTY_FUNCTION__ << ": No caps data" << std::endl; | ||
209 | 214 | return; | ||
210 | 215 | } | ||
211 | 216 | |||
212 | 217 | const gchar *mime = gst_structure_get_name(caps_data); | ||
213 | 218 | if (strstr(mime, "audio")) | ||
214 | 219 | is_missing_audio_codec = true; | ||
215 | 220 | else if (strstr(mime, "video")) | ||
216 | 221 | is_missing_video_codec = true; | ||
217 | 222 | |||
218 | 223 | std::cerr << "Missing decoder for " << mime << std::endl; | ||
219 | 180 | } | 224 | } |
220 | 181 | 225 | ||
221 | 182 | void gstreamer::Playbin::on_new_message_async(const Bus::Message& message) | 226 | void gstreamer::Playbin::on_new_message_async(const Bus::Message& message) |
222 | @@ -193,8 +237,15 @@ | |||
223 | 193 | signals.on_info(message.detail.error_warning_info); | 237 | signals.on_info(message.detail.error_warning_info); |
224 | 194 | break; | 238 | break; |
225 | 195 | case GST_MESSAGE_STATE_CHANGED: | 239 | case GST_MESSAGE_STATE_CHANGED: |
226 | 240 | if (message.source == "playbin") { | ||
227 | 241 | g_object_get(G_OBJECT(pipeline), "current-audio", &audio_stream_id, NULL); | ||
228 | 242 | g_object_get(G_OBJECT(pipeline), "current-video", &video_stream_id, NULL); | ||
229 | 243 | } | ||
230 | 196 | signals.on_state_changed(std::make_pair(message.detail.state_changed, message.source)); | 244 | signals.on_state_changed(std::make_pair(message.detail.state_changed, message.source)); |
231 | 197 | break; | 245 | break; |
232 | 246 | case GST_MESSAGE_ELEMENT: | ||
233 | 247 | process_message_element(message.message); | ||
234 | 248 | break; | ||
235 | 198 | case GST_MESSAGE_TAG: | 249 | case GST_MESSAGE_TAG: |
236 | 199 | { | 250 | { |
237 | 200 | gchar *orientation; | 251 | gchar *orientation; |
238 | @@ -598,3 +649,22 @@ | |||
239 | 598 | { | 649 | { |
240 | 599 | return file_type; | 650 | return file_type; |
241 | 600 | } | 651 | } |
242 | 652 | |||
243 | 653 | bool gstreamer::Playbin::can_play_streams() const | ||
244 | 654 | { | ||
245 | 655 | /* | ||
246 | 656 | * We do not consider that we can play the video when | ||
247 | 657 | * 1. No audio stream selected due to missing decoder | ||
248 | 658 | * 2. No video stream selected due to missing decoder | ||
249 | 659 | * 3. No stream selected at all | ||
250 | 660 | * Note that if there are several, say, audio streams, we will play the file | ||
251 | 661 | * provided that we can decode just one of them, even if there are missing | ||
252 | 662 | * audio codecs. We will also play files with only one type of stream. | ||
253 | 663 | */ | ||
254 | 664 | if ((is_missing_audio_codec && audio_stream_id == -1) || | ||
255 | 665 | (is_missing_video_codec && video_stream_id == -1) || | ||
256 | 666 | (audio_stream_id == -1 && video_stream_id == -1)) | ||
257 | 667 | return false; | ||
258 | 668 | else | ||
259 | 669 | return true; | ||
260 | 670 | } | ||
261 | 601 | 671 | ||
262 | === modified file 'src/core/media/gstreamer/playbin.h' | |||
263 | --- src/core/media/gstreamer/playbin.h 2015-06-12 18:14:05 +0000 | |||
264 | +++ src/core/media/gstreamer/playbin.h 2015-08-10 13:09:04 +0000 | |||
265 | @@ -72,6 +72,7 @@ | |||
266 | 72 | 72 | ||
267 | 73 | void on_new_message(const Bus::Message& message); | 73 | void on_new_message(const Bus::Message& message); |
268 | 74 | void on_new_message_async(const Bus::Message& message); | 74 | void on_new_message_async(const Bus::Message& message); |
269 | 75 | void process_message_element(GstMessage *message); | ||
270 | 75 | 76 | ||
271 | 76 | gstreamer::Bus& message_bus(); | 77 | gstreamer::Bus& message_bus(); |
272 | 77 | 78 | ||
273 | @@ -110,6 +111,8 @@ | |||
274 | 110 | 111 | ||
275 | 111 | MediaFileType media_file_type() const; | 112 | MediaFileType media_file_type() const; |
276 | 112 | 113 | ||
277 | 114 | bool can_play_streams() const; | ||
278 | 115 | |||
279 | 113 | GstElement* pipeline; | 116 | GstElement* pipeline; |
280 | 114 | gstreamer::Bus bus; | 117 | gstreamer::Bus bus; |
281 | 115 | MediaFileType file_type; | 118 | MediaFileType file_type; |
282 | @@ -137,6 +140,10 @@ | |||
283 | 137 | core::Signal<core::ubuntu::media::video::Dimensions> on_video_dimensions_changed; | 140 | core::Signal<core::ubuntu::media::video::Dimensions> on_video_dimensions_changed; |
284 | 138 | core::Signal<void> client_disconnected; | 141 | core::Signal<void> client_disconnected; |
285 | 139 | } signals; | 142 | } signals; |
286 | 143 | bool is_missing_audio_codec; | ||
287 | 144 | bool is_missing_video_codec; | ||
288 | 145 | gint audio_stream_id; | ||
289 | 146 | gint video_stream_id; | ||
290 | 140 | }; | 147 | }; |
291 | 141 | } | 148 | } |
292 | 142 | 149 |
A few suggestions below. I like the general direction of this approach. Are you planning on adding anything else to it or is this it?