Change who allocated the MediaPipe's shared buffer. Have the MediaPipe's implementor receive a client allocated shared buffer instead of having the client fetch a shared buffer from the implementor. BUG= R=dalesat@chromium.org Review URL: https://codereview.chromium.org/1694963002 .
diff --git a/examples/audio_play_test/play_tone.cc b/examples/audio_play_test/play_tone.cc index fdc18f2..321261b 100644 --- a/examples/audio_play_test/play_tone.cc +++ b/examples/audio_play_test/play_tone.cc
@@ -121,7 +121,6 @@ // Configure our sink for 16-bit 48KHz mono. AudioTrackConfigurationPtr cfg = AudioTrackConfiguration::New(); - cfg->max_frames = USecToBytes(BUF_DEPTH_USEC) / FRAME_BYTES; LpcmMediaTypeDetailsPtr pcm_cfg = LpcmMediaTypeDetails::New(); pcm_cfg->sample_format = LpcmSampleFormat::SIGNED_16; @@ -140,6 +139,7 @@ // proxy, pass its interface to our circular buffer helper, set up our // high/low water marks, register our callback, and start to buffer our audio. audio_pipe_.reset(new CircularBufferMediaPipeAdapter(pipe.Pass())); + audio_pipe_->Init(USecToBytes(BUF_DEPTH_USEC)); audio_pipe_->SetSignalCallback( [this](MediaResult res) -> void { GenerateToneCbk(res);
diff --git a/examples/audio_play_test/play_wav.cc b/examples/audio_play_test/play_wav.cc index 03cb392..6b89231 100644 --- a/examples/audio_play_test/play_wav.cc +++ b/examples/audio_play_test/play_wav.cc
@@ -267,7 +267,6 @@ AudioTrackConfigurationPtr cfg; cfg = AudioTrackConfiguration::New(); - cfg->max_frames = USecToFrames(BUF_DEPTH_USEC); cfg->audio_frame_ratio = tmp.numerator; cfg->media_time_ratio = tmp.denominator; @@ -296,6 +295,7 @@ // Set up our media pipe helper, configure its callback and water marks to // kick off the playback process. audio_pipe_.reset(new CircularBufferMediaPipeAdapter(media_pipe.Pass())); + audio_pipe_->Init(USecToBytes(BUF_DEPTH_USEC)); audio_pipe_->SetWatermarks(USecToBytes(BUF_HI_WATER_USEC), USecToBytes(BUF_LO_WATER_USEC)); audio_pipe_->SetSignalCallback(
diff --git a/mojo/dart/packages/mojo_services/lib/mojo/media/audio_track.mojom.dart b/mojo/dart/packages/mojo_services/lib/mojo/media/audio_track.mojom.dart index 127421d..4121604 100644 --- a/mojo/dart/packages/mojo_services/lib/mojo/media/audio_track.mojom.dart +++ b/mojo/dart/packages/mojo_services/lib/mojo/media/audio_track.mojom.dart
@@ -109,10 +109,9 @@ class AudioTrackConfiguration extends bindings.Struct { static const List<bindings.StructDataHeader> kVersions = const [ - const bindings.StructDataHeader(32, 0) + const bindings.StructDataHeader(24, 0) ]; media_types_mojom.MediaType mediaType = null; - int maxFrames = 0; int audioFrameRatio = 1; int mediaTimeRatio = 1; @@ -158,15 +157,11 @@ } if (mainDataHeader.version >= 0) { - result.maxFrames = decoder0.decodeUint64(16); + result.audioFrameRatio = decoder0.decodeUint32(16); } if (mainDataHeader.version >= 0) { - result.audioFrameRatio = decoder0.decodeUint32(24); - } - if (mainDataHeader.version >= 0) { - - result.mediaTimeRatio = decoder0.decodeUint32(28); + result.mediaTimeRatio = decoder0.decodeUint32(20); } return result; } @@ -181,21 +176,14 @@ rethrow; } try { - encoder0.encodeUint64(maxFrames, 16); - } on bindings.MojoCodecError catch(e) { - e.message = "Error encountered while encoding field " - "maxFrames of struct AudioTrackConfiguration: $e"; - rethrow; - } - try { - encoder0.encodeUint32(audioFrameRatio, 24); + encoder0.encodeUint32(audioFrameRatio, 16); } on bindings.MojoCodecError catch(e) { e.message = "Error encountered while encoding field " "audioFrameRatio of struct AudioTrackConfiguration: $e"; rethrow; } try { - encoder0.encodeUint32(mediaTimeRatio, 28); + encoder0.encodeUint32(mediaTimeRatio, 20); } on bindings.MojoCodecError catch(e) { e.message = "Error encountered while encoding field " "mediaTimeRatio of struct AudioTrackConfiguration: $e"; @@ -206,7 +194,6 @@ String toString() { return "AudioTrackConfiguration(" "mediaType: $mediaType" ", " - "maxFrames: $maxFrames" ", " "audioFrameRatio: $audioFrameRatio" ", " "mediaTimeRatio: $mediaTimeRatio" ")"; } @@ -214,7 +201,6 @@ Map toJson() { Map map = new Map(); map["mediaType"] = mediaType; - map["maxFrames"] = maxFrames; map["audioFrameRatio"] = audioFrameRatio; map["mediaTimeRatio"] = mediaTimeRatio; return map;
diff --git a/mojo/dart/packages/mojo_services/lib/mojo/media/media_pipe.mojom.dart b/mojo/dart/packages/mojo_services/lib/mojo/media/media_pipe.mojom.dart index a367aa6..ecf7139 100644 --- a/mojo/dart/packages/mojo_services/lib/mojo/media/media_pipe.mojom.dart +++ b/mojo/dart/packages/mojo_services/lib/mojo/media/media_pipe.mojom.dart
@@ -252,17 +252,16 @@ -class MediaPipeState extends bindings.Struct { +class _MediaPipeSetBufferParams extends bindings.Struct { static const List<bindings.StructDataHeader> kVersions = const [ const bindings.StructDataHeader(24, 0) ]; - static const int kMaxPayloadLen = 4611686018427387903; - core.MojoSharedBuffer payloadBuffer = null; - int payloadBufferLen = 0; + core.MojoSharedBuffer buffer = null; + int size = 0; - MediaPipeState() : super(kVersions.last.size); + _MediaPipeSetBufferParams() : super(kVersions.last.size); - static MediaPipeState deserialize(bindings.Message message) { + static _MediaPipeSetBufferParams deserialize(bindings.Message message) { var decoder = new bindings.Decoder(message); var result = decode(decoder); if (decoder.excessHandles != null) { @@ -271,11 +270,11 @@ return result; } - static MediaPipeState decode(bindings.Decoder decoder0) { + static _MediaPipeSetBufferParams decode(bindings.Decoder decoder0) { if (decoder0 == null) { return null; } - MediaPipeState result = new MediaPipeState(); + _MediaPipeSetBufferParams result = new _MediaPipeSetBufferParams(); var mainDataHeader = decoder0.decodeStructDataHeader(); if (mainDataHeader.version <= kVersions.last.version) { @@ -297,11 +296,11 @@ } if (mainDataHeader.version >= 0) { - result.payloadBuffer = decoder0.decodeSharedBufferHandle(8, false); + result.buffer = decoder0.decodeSharedBufferHandle(8, false); } if (mainDataHeader.version >= 0) { - result.payloadBufferLen = decoder0.decodeUint64(16); + result.size = decoder0.decodeUint64(16); } return result; } @@ -309,159 +308,25 @@ void encode(bindings.Encoder encoder) { var encoder0 = encoder.getStructEncoderAtOffset(kVersions.last); try { - encoder0.encodeSharedBufferHandle(payloadBuffer, 8, false); + encoder0.encodeSharedBufferHandle(buffer, 8, false); } on bindings.MojoCodecError catch(e) { e.message = "Error encountered while encoding field " - "payloadBuffer of struct MediaPipeState: $e"; + "buffer of struct _MediaPipeSetBufferParams: $e"; rethrow; } try { - encoder0.encodeUint64(payloadBufferLen, 16); + encoder0.encodeUint64(size, 16); } on bindings.MojoCodecError catch(e) { e.message = "Error encountered while encoding field " - "payloadBufferLen of struct MediaPipeState: $e"; + "size of struct _MediaPipeSetBufferParams: $e"; rethrow; } } String toString() { - return "MediaPipeState(" - "payloadBuffer: $payloadBuffer" ", " - "payloadBufferLen: $payloadBufferLen" ")"; - } - - Map toJson() { - throw new bindings.MojoCodecError( - 'Object containing handles cannot be encoded to JSON.'); - } -} - - - - -class _MediaPipeGetStateParams extends bindings.Struct { - static const List<bindings.StructDataHeader> kVersions = const [ - const bindings.StructDataHeader(8, 0) - ]; - - _MediaPipeGetStateParams() : super(kVersions.last.size); - - static _MediaPipeGetStateParams deserialize(bindings.Message message) { - var decoder = new bindings.Decoder(message); - var result = decode(decoder); - if (decoder.excessHandles != null) { - decoder.excessHandles.forEach((h) => h.close()); - } - return result; - } - - static _MediaPipeGetStateParams decode(bindings.Decoder decoder0) { - if (decoder0 == null) { - return null; - } - _MediaPipeGetStateParams result = new _MediaPipeGetStateParams(); - - var mainDataHeader = decoder0.decodeStructDataHeader(); - if (mainDataHeader.version <= kVersions.last.version) { - // Scan in reverse order to optimize for more recent versions. - for (int i = kVersions.length - 1; i >= 0; --i) { - if (mainDataHeader.version >= kVersions[i].version) { - if (mainDataHeader.size == kVersions[i].size) { - // Found a match. - break; - } - throw new bindings.MojoCodecError( - 'Header size doesn\'t correspond to known version size.'); - } - } - } else if (mainDataHeader.size < kVersions.last.size) { - throw new bindings.MojoCodecError( - 'Message newer than the last known version cannot be shorter than ' - 'required by the last known version.'); - } - return result; - } - - void encode(bindings.Encoder encoder) { - encoder.getStructEncoderAtOffset(kVersions.last); - } - - String toString() { - return "_MediaPipeGetStateParams("")"; - } - - Map toJson() { - Map map = new Map(); - return map; - } -} - - - - -class MediaPipeGetStateResponseParams extends bindings.Struct { - static const List<bindings.StructDataHeader> kVersions = const [ - const bindings.StructDataHeader(16, 0) - ]; - MediaPipeState state = null; - - MediaPipeGetStateResponseParams() : super(kVersions.last.size); - - static MediaPipeGetStateResponseParams deserialize(bindings.Message message) { - var decoder = new bindings.Decoder(message); - var result = decode(decoder); - if (decoder.excessHandles != null) { - decoder.excessHandles.forEach((h) => h.close()); - } - return result; - } - - static MediaPipeGetStateResponseParams decode(bindings.Decoder decoder0) { - if (decoder0 == null) { - return null; - } - MediaPipeGetStateResponseParams result = new MediaPipeGetStateResponseParams(); - - var mainDataHeader = decoder0.decodeStructDataHeader(); - if (mainDataHeader.version <= kVersions.last.version) { - // Scan in reverse order to optimize for more recent versions. - for (int i = kVersions.length - 1; i >= 0; --i) { - if (mainDataHeader.version >= kVersions[i].version) { - if (mainDataHeader.size == kVersions[i].size) { - // Found a match. - break; - } - throw new bindings.MojoCodecError( - 'Header size doesn\'t correspond to known version size.'); - } - } - } else if (mainDataHeader.size < kVersions.last.size) { - throw new bindings.MojoCodecError( - 'Message newer than the last known version cannot be shorter than ' - 'required by the last known version.'); - } - if (mainDataHeader.version >= 0) { - - var decoder1 = decoder0.decodePointer(8, false); - result.state = MediaPipeState.decode(decoder1); - } - return result; - } - - void encode(bindings.Encoder encoder) { - var encoder0 = encoder.getStructEncoderAtOffset(kVersions.last); - try { - encoder0.encodeStruct(state, 8, false); - } on bindings.MojoCodecError catch(e) { - e.message = "Error encountered while encoding field " - "state of struct MediaPipeGetStateResponseParams: $e"; - rethrow; - } - } - - String toString() { - return "MediaPipeGetStateResponseParams(" - "state: $state" ")"; + return "_MediaPipeSetBufferParams(" + "buffer: $buffer" ", " + "size: $size" ")"; } Map toJson() { @@ -746,7 +611,7 @@ -const int _MediaPipe_getStateName = 0; +const int _MediaPipe_setBufferName = 0; const int _MediaPipe_sendPacketName = 1; const int _MediaPipe_flushName = 2; @@ -819,9 +684,10 @@ abstract class MediaPipe { static const String serviceName = null; - dynamic getState([Function responseFactory = null]); + void setBuffer(core.MojoSharedBuffer buffer, int size); dynamic sendPacket(MediaPacket packet,[Function responseFactory = null]); dynamic flush([Function responseFactory = null]); + static const int kMaxBufferLen = 4611686018427387903; } @@ -845,26 +711,6 @@ void handleResponse(bindings.ServiceMessage message) { switch (message.header.type) { - case _MediaPipe_getStateName: - var r = MediaPipeGetStateResponseParams.deserialize( - message.payload); - if (!message.header.hasRequestId) { - proxyError("Expected a message with a valid request Id."); - return; - } - Completer c = completerMap[message.header.requestId]; - if (c == null) { - proxyError( - "Message had unknown request Id: ${message.header.requestId}"); - return; - } - completerMap.remove(message.header.requestId); - if (c.isCompleted) { - proxyError("Response completer already completed"); - return; - } - c.complete(r); - break; case _MediaPipe_sendPacketName: var r = MediaPipeSendPacketResponseParams.deserialize( message.payload); @@ -923,13 +769,15 @@ _MediaPipeProxyImpl _proxyImpl; _MediaPipeProxyCalls(this._proxyImpl); - dynamic getState([Function responseFactory = null]) { - var params = new _MediaPipeGetStateParams(); - return _proxyImpl.sendMessageWithRequestId( - params, - _MediaPipe_getStateName, - -1, - bindings.MessageHeader.kMessageExpectsResponse); + void setBuffer(core.MojoSharedBuffer buffer, int size) { + if (!_proxyImpl.isBound) { + _proxyImpl.proxyError("The Proxy is closed."); + return; + } + var params = new _MediaPipeSetBufferParams(); + params.buffer = buffer; + params.size = size; + _proxyImpl.sendMessage(params, _MediaPipe_setBufferName); } dynamic sendPacket(MediaPacket packet,[Function responseFactory = null]) { var params = new _MediaPipeSendPacketParams(); @@ -1029,11 +877,6 @@ } - MediaPipeGetStateResponseParams _MediaPipeGetStateResponseParamsFactory(MediaPipeState state) { - var mojo_factory_result = new MediaPipeGetStateResponseParams(); - mojo_factory_result.state = state; - return mojo_factory_result; - } MediaPipeSendPacketResponseParams _MediaPipeSendPacketResponseParamsFactory(MediaPipeSendResult result) { var mojo_factory_result = new MediaPipeSendPacketResponseParams(); mojo_factory_result.result = result; @@ -1052,27 +895,10 @@ } assert(_impl != null); switch (message.header.type) { - case _MediaPipe_getStateName: - var params = _MediaPipeGetStateParams.deserialize( + case _MediaPipe_setBufferName: + var params = _MediaPipeSetBufferParams.deserialize( message.payload); - var response = _impl.getState(_MediaPipeGetStateResponseParamsFactory); - if (response is Future) { - return response.then((response) { - if (response != null) { - return buildResponseWithId( - response, - _MediaPipe_getStateName, - message.header.requestId, - bindings.MessageHeader.kMessageIsResponse); - } - }); - } else if (response != null) { - return buildResponseWithId( - response, - _MediaPipe_getStateName, - message.header.requestId, - bindings.MessageHeader.kMessageIsResponse); - } + _impl.setBuffer(params.buffer, params.size); break; case _MediaPipe_sendPacketName: var params = _MediaPipeSendPacketParams.deserialize(
diff --git a/mojo/services/media/audio/interfaces/audio_track.mojom b/mojo/services/media/audio/interfaces/audio_track.mojom index 3456485..a8b8e07 100644 --- a/mojo/services/media/audio/interfaces/audio_track.mojom +++ b/mojo/services/media/audio/interfaces/audio_track.mojom
@@ -19,9 +19,6 @@ // The media type to use. MediaType media_type; - // Desired maximum buffer size, in frames of audio. - uint64 max_frames; - // Ratio of audio frames to media time ticks. // // Presentation time stamps on audio packets are expressed in units of media
diff --git a/mojo/services/media/common/cpp/circular_buffer_media_pipe_adapter.cc b/mojo/services/media/common/cpp/circular_buffer_media_pipe_adapter.cc index 4ce301f..406c50a 100644 --- a/mojo/services/media/common/cpp/circular_buffer_media_pipe_adapter.cc +++ b/mojo/services/media/common/cpp/circular_buffer_media_pipe_adapter.cc
@@ -39,11 +39,6 @@ MOJO_DCHECK(pipe_); MOJO_DCHECK(RunLoop::current()); - pipe_get_state_cbk_ = MediaPipe::GetStateCallback( - [this] (MediaPipeStatePtr state) { - HandleGetState(state.Pass()); - }); - pipe_flush_cbk_ = MediaPipe::FlushCallback( [this] () { HandleFlush(); @@ -61,12 +56,6 @@ (*thiz)->HandleSignalCallback(); } }); - - - // Begin by getting a hold of the shared buffer from our pipe over which we - // will push data. - MOJO_DCHECK(get_state_in_progress_); - pipe_->GetState(pipe_get_state_cbk_); } CircularBufferMediaPipeAdapter::~CircularBufferMediaPipeAdapter() { @@ -74,6 +63,63 @@ Cleanup(); } +void CircularBufferMediaPipeAdapter::Init(uint64_t size) { + // Double Init? Fault unless the user is asking us to set up a buffer of the + // same size. + if (buffer_handle_.is_valid()) { + if (buffer_size_ != size) { + Fault(MediaResult::BAD_STATE); + } + return; + } + + // Make the buffer + MojoResult res = CreateSharedBuffer(nullptr, size, &buffer_handle_); + if (res != MOJO_RESULT_OK) { + MOJO_LOG(ERROR) << "Failed to allocate buffer of size " << size + << " in " << __PRETTY_FUNCTION__ + << " (error " << res << ")"; + Fault(MediaResult::INSUFFICIENT_RESOURCES); + return; + } + + buffer_size_ = size; + + // Map the buffer + // + // TODO(johngro) : We really only need write access to this buffer. + // Ideally, we could request that using flags, but there does not seem to be + // a way to do this right now. + res = MapBuffer(buffer_handle_.get(), + 0, + buffer_size_, + &buffer_, + MOJO_MAP_BUFFER_FLAG_NONE); + if (res != MOJO_RESULT_OK) { + MOJO_LOG(ERROR) << "Failed to map buffer in " << __PRETTY_FUNCTION__ + << " (error " << res << ")"; + Fault(MediaResult::UNKNOWN_ERROR); + return; + } + + // Duplicate the buffer and send it to the other side of the pipe. + // + // TODO(johngro) : It would be nice if we could restrict this handle to be + // read-only. + ScopedSharedBufferHandle duplicated_handle; + res = DuplicateBuffer(buffer_handle_.get(), nullptr, &duplicated_handle); + if (res != MOJO_RESULT_OK) { + MOJO_LOG(ERROR) << "Failed to duplicate handle in " << __PRETTY_FUNCTION__ + << " (error " << res << ")"; + Fault(MediaResult::UNKNOWN_ERROR); + return; + } + + // TODO(johngro) : We should not have to send the buffer size, it should be an + // intrinsic property of the buffer itself and be query-able via the handle. + pipe_->SetBuffer(duplicated_handle.Pass(), buffer_size_); +} + void CircularBufferMediaPipeAdapter::SetSignalCallback(SignalCbk cbk) { bool schedule; signal_cbk_ = cbk; @@ -298,78 +344,12 @@ return MediaResult::OK; } -void CircularBufferMediaPipeAdapter::HandleGetState(MediaPipeStatePtr state) { - MOJO_DCHECK(state); // We must have a state structure. - MOJO_DCHECK(!buffer_); // We must not have already mapped a buffer. - MOJO_DCHECK(get_state_in_progress_); // We should be waiting for our cbk. - - // Success or failure, we are no longer waiting for our get state callback. - get_state_in_progress_ = false; - rd_ = wr_ = 0; - - // Double init? How did that happen? - if (buffer_handle_.is_valid() || (nullptr != buffer_)) { - MOJO_LOG(ERROR) << "Double init during " << __PRETTY_FUNCTION__; - Fault(MediaResult::UNKNOWN_ERROR); - return; - } - - // No shared buffer? That's a fatal error. - if (!state->payload_buffer.is_valid()) { - MOJO_LOG(ERROR) << "Null payload buffer in " << __PRETTY_FUNCTION__; - Fault(MediaResult::UNKNOWN_ERROR); - return; - } - - // stash our state - buffer_handle_ = state->payload_buffer.Pass(); - buffer_size_ = state->payload_buffer_len; - - // Sanity checks the buffer size. - if (!buffer_size_ || (buffer_size_ > MediaPipeState::kMaxPayloadLen)) { - MOJO_LOG(ERROR) << "Bad buffer size in " << __PRETTY_FUNCTION__ - << " (" << buffer_size_ << ")"; - Fault(MediaResult::BAD_STATE); - return; - } - - // Map our buffer - // - // TODO(johngro) : We really only need write access to this buffer. - // Ideally, we could request that using flags, but there does not seem to be - // a way to do this right now. - MojoResult res; - res = MapBuffer(buffer_handle_.get(), - 0, - buffer_size_, - &buffer_, - MOJO_MAP_BUFFER_FLAG_NONE); - if (res != MOJO_RESULT_OK) { - MOJO_LOG(ERROR) << "Failed to map buffer in " << __PRETTY_FUNCTION__ - << " (error " << res << ")"; - Fault(MediaResult::UNKNOWN_ERROR); - return; - } - - // Init is complete, we may be signalled now. - UpdateSignalled(); -} - void CircularBufferMediaPipeAdapter::HandleSendPacket( uint32_t seq_num, MediaPipe::SendResult result) { MediaPipe::SendPacketCallback cbk; do { - if (get_state_in_progress_) { - // If we are in the process of getting the initial state of the system, - // then something is seriously wrong. The other end of this interface is - // sending us Send callbacks while we are in the process of initializing - // (something which should be impossible) - Fault(MediaResult::PROTOCOL_ERROR); - break; - } - // There should be at least one element in the in-flight queue, and the // front of the queue's sequence number should match the sequence number of // the payload being returned to us.
diff --git a/mojo/services/media/common/cpp/circular_buffer_media_pipe_adapter.h b/mojo/services/media/common/cpp/circular_buffer_media_pipe_adapter.h index 73fe68d..679b624 100644 --- a/mojo/services/media/common/cpp/circular_buffer_media_pipe_adapter.h +++ b/mojo/services/media/common/cpp/circular_buffer_media_pipe_adapter.h
@@ -70,10 +70,34 @@ */ using SignalCbk = std::function<void(MediaResult state)>; + /** + * Constructor + * + * Create an adapter which will take ownership of the provided MediaPipe + * interface and assist in the process of generating MediaPackets and + * marshalling them to the other side of the MediaPipe. + * + * @param pipe A pointer to the MediaPipe interface which will be used as the + * target for MediaPackets. + */ explicit CircularBufferMediaPipeAdapter(MediaPipePtr pipe); + + /** + * Destructor + */ ~CircularBufferMediaPipeAdapter(); /** + * Init + * + * Allocate a shared memory buffer of the specified size and begin the process + * of marshalling it to the other side of the MediaPipe. + * + * @param size The size in bytes of the shared memory buffer to allocate. + */ + void Init(uint64_t size); + + /** * Set the signal callback for this media pipe adapter. This callback will be * called when the adapter transitions from un-signalled to signalled and has * a valid callback, or when a valid callback is assigned (via a call to @@ -205,7 +229,6 @@ }; using PacketStateQueue = std::deque<PacketState>; - void HandleGetState(MediaPipeStatePtr state); void HandleSendPacket(uint32_t seq_num, MediaPipe::SendResult result); void HandleFlush(); void HandleSignalCallback(); @@ -219,12 +242,11 @@ } bool Busy() const { - return (get_state_in_progress_ || flush_in_progress_); + return flush_in_progress_; } // Pipe interface callbacks MediaPipePtr pipe_; - MediaPipe::GetStateCallback pipe_get_state_cbk_; MediaPipe::FlushCallback pipe_flush_cbk_; Closure signalled_callback_; @@ -234,7 +256,6 @@ // State for managing signalled/un-signalled status and signal callbacks. SignalCbk signal_cbk_; - bool get_state_in_progress_ = true; bool flush_in_progress_ = false; bool fault_cbk_made_ = false; bool cbk_scheduled_ = false; @@ -246,7 +267,7 @@ ScopedSharedBufferHandle buffer_handle_; void* buffer_ = nullptr; - uint64_t buffer_size_; + uint64_t buffer_size_ = 0; uint64_t rd_, wr_; // Packet queue state
diff --git a/mojo/services/media/common/interfaces/media_pipe.mojom b/mojo/services/media/common/interfaces/media_pipe.mojom index ca5e400..7efbd2f 100644 --- a/mojo/services/media/common/interfaces/media_pipe.mojom +++ b/mojo/services/media/common/interfaces/media_pipe.mojom
@@ -61,25 +61,6 @@ // packet? }; -// MediaPipeState -// -// A small structure used by the user of a media pipe to fetch the shared buffer -// used by the media pipe to move bulk data. -// -// TODO(johngro): Right now, the only real purpose for this struture is to -// bundle the size of the buffer with the buffer handle itself. When buffer -// handles can have their properties queried directly, it can go away. See -// domokit/mojo issue #501 -struct MediaPipeState { - const uint64 kMaxPayloadLen = 0x3FFFFFFFFFFFFFFF; - - handle<shared_buffer> payload_buffer; - - // TODO(johngro) : Why do I have to send this? Why can't I just query the - // shared_buffer handle for its length? - uint64 payload_buffer_len; -}; - // MediaPipe // // An interface exposed by consumers of media which provides the means for @@ -90,6 +71,8 @@ // the media has been consumed via the SendPacket callback they provide. // Finally, the pipeline may be flushed using the Flush method. interface MediaPipe { + const uint64 kMaxBufferLen = 0x3FFFFFFFFFFFFFFF; + // An enumeration used to indicate the ultimate fate of packets sent across // the pipe using the SendPacket method. enum SendResult { @@ -97,9 +80,8 @@ FLUSHED, // Some or all of the media was flushed before being consumed. }; - // Request that a reference to the pipe's state be sent to the caller via - // callback. - GetState() => (MediaPipeState state); + // Sets the shared buffer in which sent Packet payloads will be located. + SetBuffer(handle<shared_buffer> buffer, uint64 size); // Place a media packet into the pipeline to be consumed. When the consumer // is finished with the packet, it will invoke the supplied callback to
diff --git a/services/media/audio/audio_track_impl.cc b/services/media/audio/audio_track_impl.cc index 84ecf25..a12c3af 100644 --- a/services/media/audio/audio_track_impl.cc +++ b/services/media/audio/audio_track_impl.cc
@@ -225,23 +225,9 @@ } bytes_per_frame_ *= cfg->channels; - // Overflow trying to convert from frames to bytes? - uint64_t requested_frames = configuration->max_frames; - if (requested_frames > - (std::numeric_limits<size_t>::max() / bytes_per_frame_)) { - LOG(ERROR) << "Insufficient resources to create " - << requested_frames << " frame audio buffer."; - Shutdown(); - return; - } - - size_t requested_bytes = (requested_frames * bytes_per_frame_); - - // Attempt to initialize our shared buffer and bind it to our interface - // request. - if (pipe_.Init(req.Pass(), requested_bytes) != MOJO_RESULT_OK) { - LOG(ERROR) << "Insufficient resources to create " - << requested_frames << " frame audio buffer."; + // Bind our pipe to the interface request. + if (pipe_.Init(req.Pass()) != MOJO_RESULT_OK) { + LOG(ERROR) << "Failed to media pipe to interface request."; Shutdown(); return; }
diff --git a/services/media/common/media_pipe_base.cc b/services/media/common/media_pipe_base.cc index 642c686..4a7df2d 100644 --- a/services/media/common/media_pipe_base.cc +++ b/services/media/common/media_pipe_base.cc
@@ -15,27 +15,12 @@ MediaPipeBase::~MediaPipeBase() { } -MojoResult MediaPipeBase::Init(InterfaceRequest<MediaPipe> request, - uint64_t shared_buffer_size) { +MojoResult MediaPipeBase::Init(InterfaceRequest<MediaPipe> request) { // Double init? if (IsInitialized()) { return MOJO_RESULT_ALREADY_EXISTS; } - // Valid size? - DCHECK_GT(shared_buffer_size, 0u); - DCHECK_LE(shared_buffer_size, MediaPipeState::kMaxPayloadLen); - if (!shared_buffer_size || - (shared_buffer_size > MediaPipeState::kMaxPayloadLen)) { - return MOJO_RESULT_INVALID_ARGUMENT; - } - - DCHECK(!buffer_); - buffer_ = MappedSharedBuffer::Create(shared_buffer_size); - if (buffer_ == nullptr) { - return MOJO_RESULT_UNKNOWN; - } - binding_.Bind(request.Pass()); binding_.set_connection_error_handler([this]() -> void { Reset(); @@ -44,9 +29,7 @@ } bool MediaPipeBase::IsInitialized() const { - DCHECK((!binding_.is_bound() && (buffer_ == nullptr)) || - (binding_.is_bound() && (buffer_ != nullptr))); - return !!buffer_; + return binding_.is_bound(); } void MediaPipeBase::Reset() { @@ -56,27 +39,33 @@ buffer_ = nullptr; } -void MediaPipeBase::GetState(const GetStateCallback& cbk) { - static const MojoDuplicateBufferHandleOptions options = { - .struct_size = sizeof(*this), - .flags = MOJO_DUPLICATE_BUFFER_HANDLE_OPTIONS_FLAG_NONE, - }; - - MediaPipeStatePtr state_ptr(MediaPipeState::New()); - - // If we have not been successfully initialized, send back an invalid handle - // and a zero length. - if (!buffer_) { - state_ptr->payload_buffer_len = 0; - } else { - MojoResult res = DuplicateBuffer(buffer_->handle().get(), - &options, - &state_ptr->payload_buffer); - state_ptr->payload_buffer_len = buffer_->size(); - DCHECK(MOJO_RESULT_OK == res); +void MediaPipeBase::SetBuffer(ScopedSharedBufferHandle handle, uint64_t size) { + // Double init? Close the connection. + if (buffer_) { + LOG(ERROR) << "Attempting to set a new buffer (size = " + << size + << ") on a MediaPipe which already has a buffer (size = " + << buffer_->size() + << ")"; + Reset(); + return; } - cbk.Run(state_ptr.Pass()); + // Invalid size? Close the connection. + if (!size || (size > MediaPipe::kMaxBufferLen)) { + LOG(ERROR) << "Invalid shared buffer size (size = " << size << ")"; + Reset(); + return; + } + + + // Failed to map the buffer? Close the connection. + buffer_ = MappedSharedBuffer::Create(handle.Pass(), size); + if (!buffer_) { + LOG(ERROR) << "Failed to map shared memory buffer (size = " << size << ")"; + Reset(); + return; + } } void MediaPipeBase::SendPacket(MediaPacketPtr packet, @@ -167,8 +156,9 @@ } MediaPipeBase::MappedSharedBufferPtr MediaPipeBase::MappedSharedBuffer::Create( - size_t size) { - MappedSharedBufferPtr ret(new MappedSharedBuffer(size)); + ScopedSharedBufferHandle handle, + uint64_t size) { + MappedSharedBufferPtr ret(new MappedSharedBuffer(handle.Pass(), size)); return ret->base() ? ret : nullptr; } @@ -179,35 +169,22 @@ } } -MediaPipeBase::MappedSharedBuffer::MappedSharedBuffer(size_t size) - : size_(size) { - static const MojoCreateSharedBufferOptions opt { - .struct_size = sizeof(MojoDuplicateBufferHandleOptions), - .flags = MOJO_CREATE_SHARED_BUFFER_OPTIONS_FLAG_NONE, - }; - MojoResult res; +MediaPipeBase::MappedSharedBuffer::MappedSharedBuffer( + ScopedSharedBufferHandle handle, + size_t size) + : handle_(handle.Pass()), + size_(size) { + MojoResult res = MapBuffer(handle_.get(), + 0u, + size_, + &base_, + MOJO_MAP_BUFFER_FLAG_NONE); - res = CreateSharedBuffer(&opt, size_, &handle_); - if (MOJO_RESULT_OK == res) { - // TODO(johngro) : We really only need read access to this buffer. Ideally, - // we could request that using flags, but there does not seem to be a way to - // do this right now. - res = MapBuffer(handle_.get(), - 0u, - size_, - &base_, - MOJO_MAP_BUFFER_FLAG_NONE); - - if (MOJO_RESULT_OK != res) { - LOG(ERROR) << "Failed to map shared buffer of size " << size_ - << " (res " << res << ")"; - DCHECK(base_ == nullptr); - handle_.reset(); - } - } else { - LOG(ERROR) << "Failed to create shared buffer of size " << size_ + if (MOJO_RESULT_OK != res) { + LOG(ERROR) << "Failed to map shared buffer of size " << size_ << " (res " << res << ")"; - DCHECK(!handle_.is_valid()); + DCHECK(base_ == nullptr); + handle_.reset(); } }
diff --git a/services/media/common/media_pipe_base.h b/services/media/common/media_pipe_base.h index 07bea65..9e7499f 100644 --- a/services/media/common/media_pipe_base.h +++ b/services/media/common/media_pipe_base.h
@@ -50,8 +50,7 @@ ~MediaPipeBase() override; // Initialize the internal state of the pipe (allocate resources, etc..) - MojoResult Init(InterfaceRequest<MediaPipe> request, - uint64_t shared_buffer_size); + MojoResult Init(InterfaceRequest<MediaPipe> request); bool IsInitialized() const; void Reset(); @@ -59,15 +58,16 @@ protected: class MappedSharedBuffer { public: - static MappedSharedBufferPtr Create(size_t size); + static MappedSharedBufferPtr Create(ScopedSharedBufferHandle handle, + uint64_t size); ~MappedSharedBuffer(); const ScopedSharedBufferHandle& handle() const { return handle_; } - size_t size() const { return size_; } - void* base() const { return base_; } + uint64_t size() const { return size_; } + void* base() const { return base_; } private: - explicit MappedSharedBuffer(size_t size); + MappedSharedBuffer(ScopedSharedBufferHandle handle, size_t size); ScopedSharedBufferHandle handle_; size_t size_; @@ -84,7 +84,7 @@ Binding<MediaPipe> binding_; // MediaPipe.mojom implementation. - void GetState(const GetStateCallback& cbk) final; + void SetBuffer(ScopedSharedBufferHandle handle, uint64_t size) final; void SendPacket(MediaPacketPtr packet, const SendPacketCallback& cbk) final; void Flush(const FlushCallback& cbk) final;