Merge lp:~agronick/vocal/vocal into lp:vocal

Proposed by Kyle
Status: Needs review
Proposed branch: lp:~agronick/vocal/vocal
Merge into: lp:vocal
Diff against target: 448 lines (+114/-117)
4 files modified
CMakeLists.txt (+2/-1)
src/Objects/Episode.vala (+7/-3)
src/Utils/Player.vala (+103/-112)
src/Widgets/EpisodeDetailBox.vala (+2/-1)
To merge this branch: bzr merge lp:~agronick/vocal/vocal
Reviewer Review Type Date Requested Status
Nathan Dyer Pending
Review via email: mp+261302@code.launchpad.net

Description of the change

Vocal was crashing with a segmentation fault when playing certain videos. When I looked at the URL I found that it was doing a 302 redirect and setting the Location header. In Player.vala Gst.PbUtils.DiscovererInfo.get_video_streams() did not know how to handle this and would return an empty list. When it was cast to DiscovererVideoInfo a segmentation fault is caused.

Many podcasting services use redirects for analytics and tracking. All of the videos in this RSS feed will crash Vocal unless my code is used: http://feeds2.feedburner.com/AllJupiterVideos

The new code also prevents Vocal from cashing if get_video_streams() returns an empty list for some other reason.

To post a comment you must log in.
lp:~agronick/vocal/vocal updated
296. By Kyle

Fixed segfault with certain podcasts

Revision history for this message
Kyle (agronick) wrote :

I also fixed the bug that was causing the segfault with certain feeds. This was from accessing an array out of bounds in Eppisode.vala if the dates are not formatted correctly. After checking the length of the array it was displaying "(null)" where the dates are set to appear on the sidepane. I made it so it shows an empty string instead.

Revision history for this message
Kyle (agronick) wrote :

With regard to the first bug, some of the URLs in the feed may work. If you use this URL:
http://www.podtrac.com/pts/redirect.mp4/201406.jb-dl.cdn.scaleengine.net/unfilter/2015/unfilter-0144.mp4

It is one that will definitely crash with the current code and work with the new code.

lp:~agronick/vocal/vocal updated
297. By Kyle

Merging changes

298. By Kyle

Turns out redirects were not the issue with the origional bug. The timeouts were too small. Removed redirect handling code. Kept code preventing segmentation faults. Increased timeouts.

Revision history for this message
Kyle (agronick) wrote :

Turns out redirects were not the issue with the original bug. The timeouts were too small. Removed redirect handling code. Kept code preventing segmentation faults. CMakeLists.txt can be kept in it's original state but we probably want to remove the -g option for release builds. The first fix is in Player.vala and the second one is in Episode.vala. Email me for any questions: <email address hidden>

lp:~agronick/vocal/vocal updated
299. By Kyle

New seek method makes the player a lot more responsive.

Unmerged revisions

299. By Kyle

New seek method makes the player a lot more responsive.

298. By Kyle

Turns out redirects were not the issue with the origional bug. The timeouts were too small. Removed redirect handling code. Kept code preventing segmentation faults. Increased timeouts.

297. By Kyle

Merging changes

296. By Kyle

Fixed segfault with certain podcasts

295. By Kyle

Fixed bug with redirecting URLs

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CMakeLists.txt'
2--- CMakeLists.txt 2015-04-05 16:46:38 +0000
3+++ CMakeLists.txt 2015-06-08 19:25:26 +0000
4@@ -15,7 +15,7 @@
5 list (APPEND CMAKE_MODULE_PATH ${CMAKE_SOURCE_DIR}/cmake)
6
7 file(GLOB_RECURSE sources src/*.vala)
8-
9+
10 # Some configuration
11 configure_file (${CMAKE_SOURCE_DIR}/src/config.vala.cmake ${CMAKE_SOURCE_DIR}/src/config.vala)
12
13@@ -35,6 +35,7 @@
14 --disable-warnings
15 --target-glib=2.32
16 --thread
17+ --save-temps
18 -g
19 )
20
21
22=== modified file 'src/Objects/Episode.vala'
23--- src/Objects/Episode.vala 2015-04-17 01:14:15 +0000
24+++ src/Objects/Episode.vala 2015-06-08 19:25:26 +0000
25@@ -122,15 +122,19 @@
26 offset = -2;
27 }
28
29-
30-
31+ if(fields.length < 7)
32+ return;
33+
34 // Since the pubdate is a standard, we can hard-code the values where each item is located
35-
36 int day = int.parse(fields[2 + offset]);
37 int year = int.parse(fields[4 + offset]);
38 int hour = int.parse(fields[5 + offset]);
39 int minute = int.parse(fields[6 + offset]);
40 int seconds = int.parse(fields[7 + offset]);
41+
42+ if(day == 0 || year == 0)
43+ return;
44+
45
46
47 // Determine the month number from the string as read from the pubdate
48
49=== modified file 'src/Utils/Player.vala'
50--- src/Utils/Player.vala 2015-04-04 21:51:53 +0000
51+++ src/Utils/Player.vala 2015-06-08 19:25:26 +0000
52@@ -20,18 +20,18 @@
53 using Clutter;
54 using GLib;
55 using Gst;
56-using Gst.PbUtils;
57+using Gst.PbUtils;
58
59 namespace Vocal {
60 class Player : Actor {
61
62- private static Player? player = null;
63+ private static Player? player = null;
64 public static Player? get_default (string[] args) {
65 if (player == null)
66 player = new Player(args);
67 return player;
68 }
69-
70+
71 public signal void state_changed(Gst.State new_state);
72 public signal void stream_ended();
73 public signal void additional_plugins_required(Gst.Message message);
74@@ -43,20 +43,20 @@
75
76 private MainLoop loop = new MainLoop ();
77 public signal void new_position_available();
78- private string tag_string;
79-
80+ private string tag_string;
81+
82 public bool is_currently_playing;
83-
84+
85 public Gst.State current_state;
86-
87+
88 public Episode current_episode;
89-
90+
91 private Player(string[]? args) {
92
93- /* NOTE:
94- This code is heavily influenced by the Audience source code, written by Tom Beckmann <tomjonabc@gmail.com>
95+ /* NOTE:
96+ This code is heavily influenced by the Audience source code, written by Tom Beckmann <tomjonabc@gmail.com>
97 and Corentin Noël <corentin@elementaryos.org>, available at https://launchpad.net/audience.
98- */
99+ */
100
101 // Set up the Clutter actor
102 video = new Clutter.Texture ();
103@@ -77,7 +77,7 @@
104 current_episode = null;
105
106 is_currently_playing = false;
107-
108+
109 // Check every half-second if the current media is playing, and if it is
110 // send a signal that there is a new position available
111 GLib.Timeout.add(500, () => {
112@@ -85,14 +85,14 @@
113 new_position_available();
114 return true;
115 });
116-
117+
118 }
119-
120+
121 /*
122 * Bus callback
123 */
124 private bool bus_callback (Gst.Bus bus, Gst.Message message) {
125-
126+
127 switch (message.type) {
128 case MessageType.ERROR:
129 GLib.Error err;
130@@ -115,11 +115,11 @@
131 Gst.State oldstate;
132 Gst.State newstate;
133 Gst.State pending;
134-
135-
136+
137+
138 message.parse_state_changed (out oldstate, out newstate,
139 out pending);
140-
141+
142 current_state = newstate;
143 state_changed(newstate);
144
145@@ -135,30 +135,30 @@
146
147 return true;
148 }
149-
150+
151 /*
152- * Reconfigures the video (width, positions, etc.) as needed
153- */
154+ * Reconfigures the video (width, positions, etc.) as needed
155+ */
156 public void configure_video() {
157
158- // Make sure the video has both a known width and height at this point
159- if(video_width > 0 && video_height > 0) {
160-
161- // Get the stage
162- var stage = get_stage ();
163-
164- // Calculate the videos aspect ratio
165-
166- var aspect = (float)stage.width / (float)video_width < (float)stage.height / (float)video_height ?
167- (float)stage.width / (float)video_width : (float)stage.height / (float)video_height;
168-
169- video.width = video_width * aspect;
170- video.height = video_height * aspect;
171- video.x = (stage.width - video.width) / 2;
172- video.y = (stage.height - video.height) / 2;
173+ // Make sure the video has both a known width and height at this point
174+ if(video_width > 0 && video_height > 0) {
175+
176+ // Get the stage
177+ var stage = get_stage ();
178+
179+ // Calculate the videos aspect ratio
180+
181+ var aspect = (float)stage.width / (float)video_width < (float)stage.height / (float)video_height ?
182+ (float)stage.width / (float)video_width : (float)stage.height / (float)video_height;
183+
184+ video.width = video_width * aspect;
185+ video.height = video_height * aspect;
186+ video.x = (stage.width - video.width) / 2;
187+ video.y = (stage.height - video.height) / 2;
188 }
189 }
190-
191+
192 /*
193 * Iterates through a list of tags and sets matching properties
194 */
195@@ -171,28 +171,28 @@
196 break;
197 }
198 }
199-
200+
201 /*
202 * Returns the total duration of the currently playing media
203 */
204 public int64 get_duration () {
205 int64 rv = (int64)0;
206 Gst.Format f = Gst.Format.TIME;
207-
208+
209 playbin.query_duration (f, out rv);
210-
211+
212 return rv;
213 }
214-
215+
216 /*
217 * Returns the current position of the media
218 */
219 public int64 get_position () {
220 int64 rv = (int64)0;
221 Gst.Format f = Gst.Format.TIME;
222-
223+
224 playbin.query_position (f, out rv);
225-
226+
227 return rv;
228 }
229 /*
230@@ -202,21 +202,21 @@
231 }
232
233 public double get_volume () {
234- var val = GLib.Value (typeof(double));
235- playbin.get_property ("volume", ref val);
236- return (double)val;
237- }
238+ var val = GLib.Value (typeof(double));
239+ playbin.get_property ("volume", ref val);
240+ return (double)val;
241+ }
242 */
243-
244- /*
245+
246+ /*
247 * Sets the current state to pause
248 */
249 public void pause () {
250 set_state (Gst.State.PAUSED);
251 is_currently_playing = false;
252 }
253-
254- /*
255+
256+ /*
257 * Sets the current state to playing
258 */
259 public void play () {
260@@ -225,7 +225,7 @@
261
262 is_currently_playing = true;
263 }
264-
265+
266 /*
267 * Seeks backward in the currently playing media n number of seconds
268 */
269@@ -235,13 +235,13 @@
270 int64 current = this.get_position();
271 int64 new_position = current - mil;
272 this.set_position(new_position);
273-
274+
275 new_position_available();
276-
277+
278 // If it was paused before, pause it again
279 set_state(previous);
280 }
281-
282+
283 /*
284 * Seeks forward in the currently playing media n number of seconds
285 */
286@@ -251,13 +251,13 @@
287 int64 current = this.get_position();
288 int64 new_position = current + mil;
289 this.set_position(new_position);
290-
291+
292 new_position_available();
293-
294+
295 // If it was paused before, pause it again
296 set_state(previous);
297 }
298-
299+
300 /*
301 * Sets the episode that is currently being played
302 */
303@@ -266,38 +266,44 @@
304 // Set the previous state to ready
305 set_state(Gst.State.PAUSED);
306 set_state(Gst.State.READY);
307-
308+
309 this.current_episode = episode;
310-
311+
312 // Set playbin to null
313 set_state(Gst.State.NULL);
314-
315+
316 // Set the URI
317 playbin.uri = episode.playback_uri;
318 info("Setting URI: %s".printf(episode.playback_uri));
319-
320+
321+ Gst.Bus bus = playbin.get_bus ();
322+ bus.add_watch (0, bus_callback);
323+
324 // If it's a video podcast, get the width and height and configure that information
325 if(episode.parent.content_type == MediaType.VIDEO) {
326- try {
327- var info = new Gst.PbUtils.Discoverer (10 * Gst.SECOND).discover_uri (playbin.uri);
328- var video = info.get_video_streams ();
329- if (video.data != null) {
330- var video_info = (Gst.PbUtils.DiscovererVideoInfo)video.data;
331- video_width = video_info.get_width ();
332- video_height = video_info.get_height ();
333-
334- configure_video();
335-
336- }
337- }catch(Error e) {
338- warning(e.message);
339- }
340- }
341-
342- Gst.Bus bus = playbin.get_bus ();
343- bus.add_watch (0, bus_callback);
344+
345+ var info = new Gst.PbUtils.Discoverer (50 * Gst.SECOND).discover_uri (episode.playback_uri);
346+ var video = info.get_video_streams ();
347+
348+ try {
349+
350+ video.foreach ((entry) => {
351+ DiscovererVideoInfo video_info = (DiscovererVideoInfo)entry;
352+ video_width = video_info.get_width ();
353+ video_height = video_info.get_height ();
354+ });
355+
356+ configure_video();
357+
358+ }catch(Error e) {
359+ warning(e.message);
360+ }
361+ }
362+
363+
364 }
365-
366+
367+
368 /*
369 * Change the playback rate
370 */
371@@ -307,35 +313,20 @@
372 Gst.Format.TIME, Gst.SeekFlags.SKIP,
373 Gst.SeekType.NONE, pos,
374 Gst.SeekType.NONE, get_duration ());
375- }
376-
377+ }
378+
379 /*
380 * Sets the currently playing media position
381 */
382- public void set_position (int64 pos) {
383-
384- // Slowly dial the volume back (should help avoid the crappy choppy sound when streaming)
385- for(double i = 0.1; i <= 1; i += 0.1) {
386- set_volume(1.0 - i);
387- Thread.usleep(50000);
388- }
389-
390- playbin.seek (1.0,
391- Gst.Format.TIME, Gst.SeekFlags.FLUSH,
392- Gst.SeekType.SET, pos,
393- Gst.SeekType.NONE, get_duration ());
394-
395-
396- // Let this thread sleep for .7 seconds to let GStreamer get caught up
397- Thread.usleep(700000);
398-
399- // Turn the volume back up
400- for(double j = 0.1; j <= 1; j += 0.1) {
401- set_volume(j);
402- Thread.usleep(10000);
403- }
404+ public void set_position (int64 pos) {
405+
406+ playbin.seek_simple (Gst.Format.TIME,
407+ Gst.SeekFlags.FLUSH |
408+ Gst.SeekFlags.KEY_UNIT, pos);
409+
410+
411 }
412-
413+
414 /*
415 * Set the player to a designated state
416 */
417@@ -348,11 +339,11 @@
418 playbin.set_state (s);
419 }
420
421- /*
422- * Sets the current volume
423- */
424- public void set_volume (double val) {
425- playbin.set_property ("volume", val);
426- }
427+ /*
428+ * Sets the current volume
429+ */
430+ public void set_volume (double val) {
431+ playbin.set_property ("volume", val);
432+ }
433 }
434 }
435
436=== modified file 'src/Widgets/EpisodeDetailBox.vala'
437--- src/Widgets/EpisodeDetailBox.vala 2015-04-04 19:32:30 +0000
438+++ src/Widgets/EpisodeDetailBox.vala 2015-06-08 19:25:26 +0000
439@@ -150,7 +150,8 @@
440 if(episode.datetime_released != null) {
441 release_label = new Gtk.Label(episode.datetime_released.format(" %x"));
442 } else {
443- release_label = new Gtk.Label(episode.date_released);
444+ //We must fill the label with an empty string otherwise it prints "(null)"
445+ release_label = new Gtk.Label("");
446 }
447 release_label.xalign = 0;
448 release_label.justify = Gtk.Justification.LEFT;

Subscribers

People subscribed via source and target branches

to all changes: