Added piping for sending audio packets from host to client.

remoting/protocol/content_description.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/protocol/content_description.h"
 
 #include "base/logging.h"
 #include "base/string_number_conversions.h"
 #include "remoting/base/constants.h"
 #include "remoting/protocol/authenticator.h"
 #include "third_party/libjingle/source/talk/xmllite/xmlelement.h"
 
 using buzz::QName;
 using buzz::XmlElement;
 
 namespace remoting {
 namespace protocol {
 
 const char ContentDescription::kChromotingContentName[] = "chromoting";
 
 namespace {
 
 const char kDefaultNs[] = "";
 
 // Following constants are used to format session description in XML.
 const char kDescriptionTag[] = "description";
 const char kControlTag[] = "control";
 const char kEventTag[] = "event";
 const char kVideoTag[] = "video";
+const char kAudioTag[] = "audio";
 const char kDeprecatedResolutionTag[] = "initial-resolution";
 
 const char kTransportAttr[] = "transport";
 const char kVersionAttr[] = "version";
 const char kCodecAttr[] = "codec";
 const char kDeprecatedWidthAttr[] = "width";
 const char kDeprecatedHeightAttr[] = "height";
 
 const char kStreamTransport[] = "stream";
 const char kDatagramTransport[] = "datagram";
+const char kNoneTransport[] = "none";
 
 const char kVerbatimCodec[] = "verbatim";
 const char kVp8Codec[] = "vp8";
 const char kZipCodec[] = "zip";
+const char kVorbisCodec[] = "vorbis";

[Wez, 2012/06/22 18:29:53]

Have you checked that all calling codesites cope gracefully with unexpected
codecs, e.g. if I specify a vorbis video tag, will the host correctly reject the
config?

[kxing, 2012/06/22 19:12:31]

I think so:

remoting/client/rectangle_update_decoder.cc checks for it correctly
remoting/host/chromoting_host.cc checks for it correctly
remoting/host/simple_host_process.cc checks for it correctly
remoting/protocol/video_reader.cc checks for it correctly

Let me know if I'm missing something.

 
 const char* GetTransportName(ChannelConfig::TransportType type) {
   switch (type) {
     case ChannelConfig::TRANSPORT_STREAM:
       return kStreamTransport;
     case ChannelConfig::TRANSPORT_DATAGRAM:
       return kDatagramTransport;
+    case ChannelConfig::TRANSPORT_NONE:
+      return kNoneTransport;
   }
   NOTREACHED();
   return NULL;
 }
 
 const char* GetCodecName(ChannelConfig::Codec type) {
   switch (type) {
     case ChannelConfig::CODEC_VERBATIM:
       return kVerbatimCodec;
     case ChannelConfig::CODEC_VP8:
       return kVp8Codec;
     case ChannelConfig::CODEC_ZIP:
       return kZipCodec;
+    case ChannelConfig::CODEC_VORBIS:
+      return kVorbisCodec;
     default:
       break;
   }
   NOTREACHED();
   return NULL;
 }
 
 
 // Format a channel configuration tag for chromotocol session description,
 // e.g. for video channel:
@@ skipping 16 matching lines @@
 
   return result;
 }
 
 bool ParseTransportName(const std::string& value,
                         ChannelConfig::TransportType* transport) {
   if (value == kStreamTransport) {
     *transport = ChannelConfig::TRANSPORT_STREAM;
   } else if (value == kDatagramTransport) {
     *transport = ChannelConfig::TRANSPORT_DATAGRAM;
+  } else if (value == kNoneTransport) {
+    *transport = ChannelConfig::TRANSPORT_NONE;
   } else {
     return false;
   }
   return true;
 }
 
 bool ParseCodecName(const std::string& value, ChannelConfig::Codec* codec) {
   if (value == kVerbatimCodec) {
     *codec = ChannelConfig::CODEC_VERBATIM;
   } else if (value == kVp8Codec) {
     *codec = ChannelConfig::CODEC_VP8;
   } else if (value == kZipCodec) {
     *codec = ChannelConfig::CODEC_ZIP;
+  } else if (value == kVorbisCodec) {
+    *codec = ChannelConfig::CODEC_VORBIS;
   } else {
     return false;
   }
   return true;
 }
 
 // Returns false if the element is invalid.
 bool ParseChannelConfig(const XmlElement* element, bool codec_required,
                         ChannelConfig* config) {
   if (!ParseTransportName(element->Attr(QName(kDefaultNs, kTransportAttr)),
@@ skipping 33 matching lines @@
   scoped_ptr<XmlElement> message(new XmlElement(*authenticator_message_));
   return new ContentDescription(candidate_config_->Clone(), message.Pass());
 }
 
 // ToXml() creates content description for chromoting session. The
 // description looks as follows:
 //   <description xmlns="google:remoting">
 //     <control transport="stream" version="1" />
 //     <event transport="datagram" version="1" />
 //     <video transport="stream" codec="vp8" version="1" />
+//     <audio transport="stream" codec="vorbis" version="1" />
 //     <authentication>
 //      Message created by Authenticator implementation.
 //     </authentication>
 //   </description>
 //
 XmlElement* ContentDescription::ToXml() const {
   XmlElement* root = new XmlElement(
       QName(kChromotingXmlNamespace, kDescriptionTag), true);
 
   std::vector<ChannelConfig>::const_iterator it;
 
   for (it = config()->control_configs().begin();
        it != config()->control_configs().end(); ++it) {
     root->AddElement(FormatChannelConfig(*it, kControlTag));
   }
 
   for (it = config()->event_configs().begin();
        it != config()->event_configs().end(); ++it) {
     root->AddElement(FormatChannelConfig(*it, kEventTag));
   }
 
   for (it = config()->video_configs().begin();
        it != config()->video_configs().end(); ++it) {
     root->AddElement(FormatChannelConfig(*it, kVideoTag));
   }
 
+  for (it = config()->audio_configs().begin();
+       it != config()->audio_configs().end(); ++it) {
+    root->AddElement(FormatChannelConfig(*it, kAudioTag));
+  }
+
   // Older endpoints require an initial-resolution tag, but otherwise ignore it.
   XmlElement* resolution_tag = new XmlElement(
       QName(kChromotingXmlNamespace, kDeprecatedResolutionTag));
   resolution_tag->AddAttr(QName(kDefaultNs, kDeprecatedWidthAttr), "640");
   resolution_tag->AddAttr(QName(kDefaultNs, kDeprecatedHeightAttr), "480");
   root->AddElement(resolution_tag);
 
   if (authenticator_message_.get()) {
     DCHECK(Authenticator::IsAuthenticatorMessage(authenticator_message_.get()));
     root->AddElement(new XmlElement(*authenticator_message_));
@@ skipping 36 matching lines @@
     QName video_tag(kChromotingXmlNamespace, kVideoTag);
     child = element->FirstNamed(video_tag);
     while (child) {
       ChannelConfig channel_config;
       if (!ParseChannelConfig(child, true, &channel_config))
         return NULL;
       config->mutable_video_configs()->push_back(channel_config);
       child = child->NextNamed(video_tag);
     }
 
+    // <audio> tags.
+    QName audio_tag(kChromotingXmlNamespace, kAudioTag);
+    child = element->FirstNamed(audio_tag);
+    if (!child) {
+      // If there's no mention of audio, implicitly assume
+      // TRANSPORT_NONE for the audio_channel.
+      ChannelConfig no_audio(ChannelConfig::TRANSPORT_NONE,
+                             kDefaultStreamVersion,
+                             ChannelConfig::CODEC_UNDEFINED);
+      config->mutable_audio_configs()->push_back(no_audio);
+    }
+    while (child) {
+      ChannelConfig channel_config;
+      if (!ParseChannelConfig(child, false, &channel_config))
+        return NULL;
+      config->mutable_audio_configs()->push_back(channel_config);
+      child = child->NextNamed(audio_tag);
+    }

[Wez, 2012/06/22 18:29:53]

nit: We've got four all-but-identical blocks of code here; can we fold them into
a single block w/ iterator over the known channel names?  Since calling code
can't cope with "none" for anything but the audio-channel, the audio-channel
special-case can be handled by checking the number of audio_configs() after
we've processed the XML, and adding a "none" config if that is zero.

[kxing, 2012/06/22 19:12:31]

This might be a bit tricky, since video tags require a codec (the second
parameter to ParseChannelConfig is true). Do you think doing special case
handling in the middle of the loop is worth it?

[Wez, 2012/06/22 21:14:06]

Good point.  You also want audio tags to have a codec property, I think, but
you're passing false to ParseChannelConfig() for those right now.

You'd need a structure to iterate over that contains the channel name and a
pointer to the mutable configs structure for it, so adding a needs-codec flag to
that would seem reasonable. Alternatively you could just have a static method
that, given a channel name, returns true if that channel needs a codec, false
otherwise.

[kxing, 2012/06/22 21:21:37]

Sure. I'll do that in https://code.google.com/p/chromium/issues/detail?id=134222

+
     scoped_ptr<XmlElement> authenticator_message;
     child = Authenticator::FindAuthenticatorMessage(element);
     if (child)
       authenticator_message.reset(new XmlElement(*child));
 
     return new ContentDescription(config.Pass(), authenticator_message.Pass());
   }
   LOG(ERROR) << "Invalid description: " << element->Str();
   return NULL;
 }
 
 }  // namespace protocol
 }  // namespace remoting