From 7d8b15ed75b18f37149ee18c142fd4cd1fda0c11 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 28 Aug 2018 14:47:01 -0700 Subject: [PATCH] move valid position check to packet processing --- .../src/audio/AudioMixerClientData.cpp | 78 +++++++++++++++---- .../src/audio/AudioMixerClientData.h | 2 + .../src/audio/AudioMixerSlave.cpp | 9 +-- .../src/audio/AvatarAudioStream.cpp | 10 +-- .../src/audio/AvatarAudioStream.h | 2 + libraries/audio/src/InboundAudioStream.h | 2 + libraries/audio/src/InjectedAudioStream.cpp | 2 +- libraries/audio/src/InjectedAudioStream.h | 2 + libraries/audio/src/PositionalAudioStream.cpp | 11 --- libraries/audio/src/PositionalAudioStream.h | 7 +- 10 files changed, 83 insertions(+), 42 deletions(-) diff --git a/assignment-client/src/audio/AudioMixerClientData.cpp b/assignment-client/src/audio/AudioMixerClientData.cpp index e0b156473a..40d5cbfcc5 100644 --- a/assignment-client/src/audio/AudioMixerClientData.cpp +++ b/assignment-client/src/audio/AudioMixerClientData.cpp @@ -13,6 +13,8 @@ #include +#include + #include #include @@ -332,18 +334,65 @@ int AudioMixerClientData::parseData(ReceivedMessage& message) { return 0; } +bool AudioMixerClientData::containsValidPosition(ReceivedMessage& message) const { + static const int SEQUENCE_NUMBER_BYTES = sizeof(quint16); + + auto posBefore = message.getPosition(); + + message.seek(SEQUENCE_NUMBER_BYTES); + + // skip over the codec string + message.readString(); + + switch (message.getType()) { + case PacketType::MicrophoneAudioNoEcho: + case PacketType::MicrophoneAudioWithEcho: { + // skip over the stereo flag + message.seek(message.getPosition() + sizeof(ChannelFlag)); + break; + } + case PacketType::SilentAudioFrame: { + // skip the number of silent samples + message.seek(message.getPosition() + sizeof(SilentSamplesBytes)); + break; + } + case PacketType::InjectAudio: { + // skip the stream ID, stereo flag, and loopback flag + message.seek(message.getPosition() + NUM_STREAM_ID_BYTES + sizeof(ChannelFlag) + sizeof(LoopbackFlag)); + } + default: + Q_UNREACHABLE(); + break; + } + + glm::vec3 peekPosition; + message.readPrimitive(&peekPosition); + + // reset the position the message was at before we were called + message.seek(posBefore); + + if (glm::any(glm::isnan(peekPosition))) { + return false; + } + + return true; +} + void AudioMixerClientData::processStreamPacket(ReceivedMessage& message, ConcurrentAddedStreams &addedStreams) { + + if (!containsValidPosition(message)) { + qDebug() << "Refusing to process audio stream from" << message.getSourceID() << "with invalid position"; + return; + } + SharedStreamPointer matchingStream; auto packetType = message.getType(); bool newStream = false; if (packetType == PacketType::MicrophoneAudioWithEcho - || packetType == PacketType::ReplicatedMicrophoneAudioWithEcho || packetType == PacketType::MicrophoneAudioNoEcho - || packetType == PacketType::ReplicatedMicrophoneAudioNoEcho - || packetType == PacketType::SilentAudioFrame - || packetType == PacketType::ReplicatedSilentAudioFrame) { + || packetType == PacketType::SilentAudioFrame) { QWriteLocker writeLocker { &_streamsLock }; @@ -355,7 +404,7 @@ void AudioMixerClientData::processStreamPacket(ReceivedMessage& message, Concurr // we don't have a mic stream yet, so add it // hop past the sequence number that leads the packet - message.seek(sizeof(quint16)); + message.seek(sizeof(StreamSequenceNumber)); // pull the codec string from the packet auto codecString = message.readString(); @@ -363,11 +412,11 @@ void AudioMixerClientData::processStreamPacket(ReceivedMessage& message, Concurr // determine if the stream is stereo or not bool isStereo; if (packetType == PacketType::SilentAudioFrame || packetType == PacketType::ReplicatedSilentAudioFrame) { - quint16 numSilentSamples; + SilentSamplesBytes numSilentSamples; message.readPrimitive(&numSilentSamples); isStereo = numSilentSamples == AudioConstants::NETWORK_FRAME_SAMPLES_STEREO; } else { - quint8 channelFlag; + ChannelFlag channelFlag; message.readPrimitive(&channelFlag); isStereo = channelFlag == 1; } @@ -395,17 +444,15 @@ void AudioMixerClientData::processStreamPacket(ReceivedMessage& message, Concurr } writeLocker.unlock(); - } else if (packetType == PacketType::InjectAudio - || packetType == PacketType::ReplicatedInjectAudio) { + } else if (packetType == PacketType::InjectAudio) { + // this is injected audio - // grab the stream identifier for this injected audio - message.seek(sizeof(quint16)); + // skip the sequence number and codec string and grab the stream identifier for this injected audio + message.seek(sizeof(StreamSequenceNumber)); + message.readString(); QUuid streamIdentifier = QUuid::fromRfc4122(message.readWithoutCopy(NUM_BYTES_RFC4122_UUID)); - bool isStereo; - message.readPrimitive(&isStereo); - QWriteLocker writeLock { &_streamsLock }; auto streamIt = std::find_if(_audioStreams.begin(), _audioStreams.end(), [&streamIdentifier](const SharedStreamPointer& stream) { @@ -413,6 +460,9 @@ void AudioMixerClientData::processStreamPacket(ReceivedMessage& message, Concurr }); if (streamIt == _audioStreams.end()) { + bool isStereo; + message.readPrimitive(&isStereo); + // we don't have this injected stream yet, so add it auto injectorStream = new InjectedAudioStream(streamIdentifier, isStereo, AudioMixer::getStaticJitterFrames()); diff --git a/assignment-client/src/audio/AudioMixerClientData.h b/assignment-client/src/audio/AudioMixerClientData.h index 7de5309717..96971cb0ba 100644 --- a/assignment-client/src/audio/AudioMixerClientData.h +++ b/assignment-client/src/audio/AudioMixerClientData.h @@ -172,6 +172,8 @@ private: void setGainForAvatar(QUuid nodeID, uint8_t gain); + bool containsValidPosition(ReceivedMessage& message) const; + MixableStreamsVector _mixableStreams; quint16 _outgoingMixedAudioSequenceNumber; diff --git a/assignment-client/src/audio/AudioMixerSlave.cpp b/assignment-client/src/audio/AudioMixerSlave.cpp index 547043c0f0..94d7d06b8c 100644 --- a/assignment-client/src/audio/AudioMixerSlave.cpp +++ b/assignment-client/src/audio/AudioMixerSlave.cpp @@ -135,12 +135,6 @@ bool AudioMixerSlave::prepareMix(const SharedNodePointer& listener) { AvatarAudioStream* listenerAudioStream = static_cast(listener->getLinkedData())->getAvatarAudioStream(); AudioMixerClientData* listenerData = static_cast(listener->getLinkedData()); - // if we received an invalid position from this listener, then refuse to make them a mix - // because we don't know how to do it properly - if (!listenerAudioStream->hasValidPosition()) { - return false; - } - // zero out the mix for this listener memset(_mixSamples, 0, sizeof(_mixSamples)); @@ -195,6 +189,7 @@ bool AudioMixerSlave::prepareMix(const SharedNodePointer& listener) { auto it = mixableStreams.begin(); auto end = mixableStreams.end(); while (it != end) { + // check if this node (and therefore all of the node's streams) has been removed auto& nodeIDStreamID = it->nodeStreamID; auto matchedRemovedNode = std::find(_sharedData.removedNodes.cbegin(), _sharedData.removedNodes.cend(), @@ -279,7 +274,7 @@ bool AudioMixerSlave::prepareMix(const SharedNodePointer& listener) { } else { // we're throttling, so we need to update the approximate volume for any un-skipped streams // unless this is simply for an echo (in which case the approx volume is 1.0) - if (!it->skippedStream) { + if (!it->skippedStream && it->positionalStream->getLastPopOutputTrailingLoudness() > 0.0f) { if (it->positionalStream != listenerAudioStream) { // approximate the gain float gain = approximateGain(*listenerAudioStream, *(it->positionalStream)); diff --git a/assignment-client/src/audio/AvatarAudioStream.cpp b/assignment-client/src/audio/AvatarAudioStream.cpp index 22ea8c0617..1b3ca9a8b1 100644 --- a/assignment-client/src/audio/AvatarAudioStream.cpp +++ b/assignment-client/src/audio/AvatarAudioStream.cpp @@ -23,9 +23,9 @@ int AvatarAudioStream::parseStreamProperties(PacketType type, const QByteArray& if (type == PacketType::SilentAudioFrame) { const char* dataAt = packetAfterSeqNum.constData(); - quint16 numSilentSamples = *(reinterpret_cast(dataAt)); - readBytes += sizeof(quint16); - numAudioSamples = (int)numSilentSamples; + SilentSamplesBytes numSilentSamples = *(reinterpret_cast(dataAt)); + readBytes += sizeof(SilentSamplesBytes); + numAudioSamples = (int) numSilentSamples; // read the positional data readBytes += parsePositionalData(packetAfterSeqNum.mid(readBytes)); @@ -34,9 +34,9 @@ int AvatarAudioStream::parseStreamProperties(PacketType type, const QByteArray& _shouldLoopbackForNode = (type == PacketType::MicrophoneAudioWithEcho); // read the channel flag - quint8 channelFlag = packetAfterSeqNum.at(readBytes); + ChannelFlag channelFlag = packetAfterSeqNum.at(readBytes); bool isStereo = channelFlag == 1; - readBytes += sizeof(quint8); + readBytes += sizeof(ChannelFlag); // if isStereo value has changed, restart the ring buffer with new frame size if (isStereo != _isStereo) { diff --git a/assignment-client/src/audio/AvatarAudioStream.h b/assignment-client/src/audio/AvatarAudioStream.h index 497e522922..de9577099e 100644 --- a/assignment-client/src/audio/AvatarAudioStream.h +++ b/assignment-client/src/audio/AvatarAudioStream.h @@ -16,6 +16,8 @@ #include "PositionalAudioStream.h" +using SilentSamplesBytes = quint16; + class AvatarAudioStream : public PositionalAudioStream { public: AvatarAudioStream(bool isStereo, int numStaticJitterFrames = -1); diff --git a/libraries/audio/src/InboundAudioStream.h b/libraries/audio/src/InboundAudioStream.h index ecd1a118f9..5ff9e2c84c 100644 --- a/libraries/audio/src/InboundAudioStream.h +++ b/libraries/audio/src/InboundAudioStream.h @@ -30,6 +30,8 @@ // Audio Env bitset const int HAS_REVERB_BIT = 0; // 1st bit +using StreamSequenceNumber = quint16; + class InboundAudioStream : public NodeData { Q_OBJECT diff --git a/libraries/audio/src/InjectedAudioStream.cpp b/libraries/audio/src/InjectedAudioStream.cpp index 2f357416f2..4c598fe6d1 100644 --- a/libraries/audio/src/InjectedAudioStream.cpp +++ b/libraries/audio/src/InjectedAudioStream.cpp @@ -50,7 +50,7 @@ int InjectedAudioStream::parseStreamProperties(PacketType type, } // pull the loopback flag and set our boolean - uchar shouldLoopback; + LoopbackFlag shouldLoopback; packetStream >> shouldLoopback; _shouldLoopbackForNode = (shouldLoopback == 1); diff --git a/libraries/audio/src/InjectedAudioStream.h b/libraries/audio/src/InjectedAudioStream.h index 75b1c0b236..990ea81272 100644 --- a/libraries/audio/src/InjectedAudioStream.h +++ b/libraries/audio/src/InjectedAudioStream.h @@ -16,6 +16,8 @@ #include "PositionalAudioStream.h" +using LoopbackFlag = uchar; + class InjectedAudioStream : public PositionalAudioStream { public: InjectedAudioStream(const QUuid& streamIdentifier, bool isStereo, int numStaticJitterFrames = -1); diff --git a/libraries/audio/src/PositionalAudioStream.cpp b/libraries/audio/src/PositionalAudioStream.cpp index 956d2bc105..4161b66060 100644 --- a/libraries/audio/src/PositionalAudioStream.cpp +++ b/libraries/audio/src/PositionalAudioStream.cpp @@ -14,7 +14,6 @@ #include -#include #include #include @@ -78,16 +77,6 @@ int PositionalAudioStream::parsePositionalData(const QByteArray& positionalByteA QDataStream packetStream(positionalByteArray); packetStream.readRawData(reinterpret_cast(&_position), sizeof(_position)); - - // if the client sends us a bad position, flag it so that we don't consider this stream for mixing - if (glm::isnan(_position.x) || glm::isnan(_position.y) || glm::isnan(_position.z)) { - HIFI_FDEBUG("PositionalAudioStream unpacked invalid position for node" << uuidStringWithoutCurlyBraces(getNodeID()) ); - - _hasValidPosition = false; - } else { - _hasValidPosition = true; - } - packetStream.readRawData(reinterpret_cast(&_orientation), sizeof(_orientation)); packetStream.readRawData(reinterpret_cast(&_avatarBoundingBoxCorner), sizeof(_avatarBoundingBoxCorner)); packetStream.readRawData(reinterpret_cast(&_avatarBoundingBoxScale), sizeof(_avatarBoundingBoxScale)); diff --git a/libraries/audio/src/PositionalAudioStream.h b/libraries/audio/src/PositionalAudioStream.h index f11a43f93f..01a714aeb4 100644 --- a/libraries/audio/src/PositionalAudioStream.h +++ b/libraries/audio/src/PositionalAudioStream.h @@ -20,6 +20,7 @@ const int AUDIOMIXER_INBOUND_RING_BUFFER_FRAME_CAPACITY = 100; using StreamID = QUuid; +const int NUM_STREAM_ID_BYTES = NUM_BYTES_RFC4122_UUID; struct NodeIDStreamID { QUuid nodeID; @@ -34,6 +35,8 @@ struct NodeIDStreamID { } }; +using ChannelFlag = quint8; + class PositionalAudioStream : public InboundAudioStream { Q_OBJECT public: @@ -66,8 +69,6 @@ public: const glm::vec3& getAvatarBoundingBoxCorner() const { return _avatarBoundingBoxCorner; } const glm::vec3& getAvatarBoundingBoxScale() const { return _avatarBoundingBoxScale; } - bool hasValidPosition() const { return _hasValidPosition; } - using IgnoreBox = AABox; // called from single AudioMixerSlave while processing packets for node @@ -106,8 +107,6 @@ protected: float _quietestFrameLoudness; int _frameCounter; - bool _hasValidPosition { false }; - bool _isIgnoreBoxEnabled { false }; IgnoreBox _ignoreBox; };