GNOME Bugzilla – Bug 649931
[apexsink] Allow to play to new, generation 2 AirTunes (AirPlay) hardware
Last modified: 2011-05-19 06:08:19 UTC
Created attachment 187604 [details] [review] Patch against gst-plugins-bad Git master, as of approx. May 5th 2011 at 22:00 CST It seems that new Apple devices expect a slightly different protocol when receiving an AirTunes stream. I have written a patch that allow apexsink to stream to these devices. To support this, the plugin now has two additional properties: generation and transport-protocol. With this patch, I am able to stream to AirServer, a software implementation of the newer protocol. There are some issues remaining, probably having to do with the use of usleep (notice the comment in the patch): 1. For some reason, no audio plays unless I specify "--gst-debug=apexsink:5" on the gst-launch command line. Very odd. Perhaps printing to the console slows down the timing of things. 2. I hear a skip every 6-40 seconds.
Review of attachment 187604 [details] [review]: So the new generation uses UDP? You should make sure that you don't send too large packets from the sink then, TCP will handle that correctly by using fragmentation but for UDP you have to do it yourself ::: ext/apexsink/gstapexsink.c @@ +119,3 @@ + static GEnumValue generation[] = { + {GST_APEX_GENERATION_ONE, "GST_APEX_GENERATION_ONE", "First generation (e.g., original AirPort Express)"}, + {GST_APEX_GENERATION_TWO, "GST_APEX_GENERATION_TWO", "Second generation (e.g., Apple TV v2)"}, Please use something else for the name (the first string), e.g. generation-one @@ +136,3 @@ + static GEnumValue transport_protocol[] = { + {GST_APEX_TCP, "GST_APEX_TCP", "TCP"}, + {GST_APEX_UDP, "GST_APEX_UDP", "UDP"}, Please use something else for the name (the first string), e.g. udp and tcp @@ +407,3 @@ + GST_INFO_OBJECT (sink, "ApEx transport protocol set to \"%d\"", sink->transport_protocol); + } else { + G_OBJECT_WARN_INVALID_PSPEC (object, "transport protocol", prop_id, pspec); Instead of this warning you should do nothing and use a GST_WARNING_OBJECT() that says that the property can only be changed in NULL state. @@ +575,3 @@ + */ + usleep ((gulong) ((length * 1000000.) / (GST_APEX_RAOP_BITRATE * + GST_APEX_RAOP_BYTES_PER_SAMPLE))); Instead of sleeping it's probably a good idea to use the GstClock API here, which allows interruptions and everything.
BTW, I realise that this is mainly for testing, but the airport express support that should actually be used is the one in PulseAudio. http://git.zx2c4.com/pulseaudio-raop2/ has a base for RAOP2 support in PulseAudio.
(In reply to comment #2) > BTW, I realise that this is mainly for testing, but the airport express support > that should actually be used is the one in PulseAudio. I agree, for many desktop applications. However, there are other uses where GStreamer may be more appropriate or convenient.
> So the new generation uses UDP? You should make sure that you don't send too > large packets from the sink then, TCP will handle that correctly by using > fragmentation but for UDP you have to do it yourself The new generation uses either TCP or UDP. The protocol supported is advertised using mDNS/SD. Either way, the packet size is fixed at 12 (RTSP header) + 3 (ALAC header) + 1408 (data). > ::: ext/apexsink/gstapexsink.c > @@ +119,3 @@ > + static GEnumValue generation[] = { > + {GST_APEX_GENERATION_ONE, "GST_APEX_GENERATION_ONE", "First generation > (e.g., original AirPort Express)"}, > + {GST_APEX_GENERATION_TWO, "GST_APEX_GENERATION_TWO", "Second generation > (e.g., Apple TV v2)"}, > > Please use something else for the name (the first string), e.g. generation-one Done in second patch. > @@ +136,3 @@ > + static GEnumValue transport_protocol[] = { > + {GST_APEX_TCP, "GST_APEX_TCP", "TCP"}, > + {GST_APEX_UDP, "GST_APEX_UDP", "UDP"}, > > Please use something else for the name (the first string), e.g. udp and tcp Done in second patch. > @@ +407,3 @@ > + GST_INFO_OBJECT (sink, "ApEx transport protocol set to \"%d\"", > sink->transport_protocol); > + } else { > + G_OBJECT_WARN_INVALID_PSPEC (object, "transport protocol", prop_id, > pspec); > > Instead of this warning you should do nothing and use a GST_WARNING_OBJECT() > that says that the property can only be changed in NULL state. Done in second patch. > @@ +575,3 @@ > + */ > + usleep ((gulong) ((length * 1000000.) / (GST_APEX_RAOP_BITRATE * > + GST_APEX_RAOP_BYTES_PER_SAMPLE))); > > Instead of sleeping it's probably a good idea to use the GstClock API here, > which allows interruptions and everything. Done in second patch. I am still working on the timing issues noted in the original report.
Created attachment 187886 [details] [review] Patch against gst-plugins-bad Git master, as of approx. May 5th 2011 at 22:00 CST
Review of attachment 187886 [details] [review]: ::: ext/apexsink/gstapexsink.c @@ +579,3 @@ + (GstClockTime) (gst_clock_get_time(apexsink->clock) + ((length * 1000000000.) + / (GST_APEX_RAOP_BITRATE * GST_APEX_RAOP_BYTES_PER_SAMPLE)))); + gst_clock_id_wait (clock_id, NULL); Looks good but to make this interruptible you have to store the clock_id somewhere and cancel it in the state change function when going from PAUSED to READY (before chaining up to the parent class). Everything else looks good now and I'll commit it after this change
Created attachment 188065 [details] [review] Patch against gst-plugins-bad Git master, as of approx. May 5th 2011 at 22:00 CST This version of the patch should address comment #6. It also fixes the issue where audio would not play unless used with --gst-debug=apexsink:5.
Comment on attachment 188065 [details] [review] Patch against gst-plugins-bad Git master, as of approx. May 5th 2011 at 22:00 CST No it doesn't, you need to store the clockid somewhere in the apexsink instance and cancel it when changing the state from PAUSED to READY. I don't see any changes in that part of the code
Created attachment 188075 [details] [review] Patch against gst-plugins-bad Git master, as of approx. May 18th 2011 at 15:50 CST Unschedule clock ID on state change from PAUSED to READY.
commit a08290f9314fc01c6d8f0ad7165d2ba360cb0409 Author: W. Michael Petullo <mike@flyn.org> Date: Thu May 19 08:05:14 2011 +0200 apexsink: Add support for generation 2 AirTunes hardware Fixes bug #649931.