From 79949bdc7960c7bd81732537aace9acacfe847b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fernando=20Jim=C3=A9nez=20Moreno?= Date: Fri, 22 Dec 2017 16:26:07 +0100 Subject: [PATCH] Use struct callbacks instead of closures for AppendData and RangeRemoval --- .../gecko/glue/GeckoMediaSourceBuffer.cpp | 15 ++--- gecko-media/gecko/glue/SourceBuffer.cpp | 55 ++++++---------- .../glue/include/GeckoMediaSourceBuffer.h | 15 ++--- gecko-media/gecko/glue/include/SourceBuffer.h | 23 ++----- .../glue/include/SourceBufferAttributes.h | 12 ++++ gecko-media/src/lib.rs | 42 +++++++------ gecko-media/src/mse/sourcebuffer.rs | 62 +++++-------------- 7 files changed, 82 insertions(+), 142 deletions(-) diff --git a/gecko-media/gecko/glue/GeckoMediaSourceBuffer.cpp b/gecko-media/gecko/glue/GeckoMediaSourceBuffer.cpp index ae72118..41a3dfa 100644 --- a/gecko-media/gecko/glue/GeckoMediaSourceBuffer.cpp +++ b/gecko-media/gecko/glue/GeckoMediaSourceBuffer.cpp @@ -57,16 +57,11 @@ GeckoMedia_SourceBuffer_EvictData(size_t aId, size_t aLength, bool* aBufferFull) void GeckoMedia_SourceBuffer_AppendData(size_t aId, const uint8_t* aData, - size_t aLength, - success_callback_t aSuccessCb, - void* aSuccessCbContext, - error_callback_t aErrorCb, - void* aErrorCbContext) + size_t aLength) { IMPL_GECKO_MEDIA_REFLECTOR_GET(GeckoMediaSourceBuffer) - reflector->mSourceBuffer->AppendData( - aData, aLength, aSuccessCb, aSuccessCbContext, aErrorCb, aErrorCbContext); + reflector->mSourceBuffer->AppendData(aData, aLength); } void @@ -88,13 +83,11 @@ GeckoMedia_SourceBuffer_ResetParserState(size_t aId) void GeckoMedia_SourceBuffer_RangeRemoval(size_t aId, double aStart, - double aEnd, - success_callback_t aSuccessCb, - void* aSuccessCbContext) + double aEnd) { IMPL_GECKO_MEDIA_REFLECTOR_GET(GeckoMediaSourceBuffer) - reflector->mSourceBuffer->RangeRemoval(aStart, aEnd, aSuccessCb, aSuccessCbContext); + reflector->mSourceBuffer->RangeRemoval(aStart, aEnd); } SourceBuffer* diff --git a/gecko-media/gecko/glue/SourceBuffer.cpp b/gecko-media/gecko/glue/SourceBuffer.cpp index 6e65d87..06828f2 100644 --- a/gecko-media/gecko/glue/SourceBuffer.cpp +++ b/gecko-media/gecko/glue/SourceBuffer.cpp @@ -73,45 +73,33 @@ SourceBuffer::EvictData(size_t aLength, bool* aBufferFull) } void -SourceBuffer::AppendData(const uint8_t* aData, - size_t aLength, - success_callback_t aSuccessCb, - void* aSuccessCbContext, - error_callback_t aErrorCb, - void* aErrorCbContext) +SourceBuffer::AppendData(const uint8_t* aData, size_t aLength) { MSE_DEBUG("AppendData(aLength=%zu)", aLength); RefPtr data = new MediaByteBuffer(); if (NS_WARN_IF(!data->AppendElements(aData, aLength, fallible))) { - return (*aErrorCb)(aErrorCbContext, - uint32_t(NS_ERROR_DOM_QUOTA_EXCEEDED_ERR)); + return mCurrentAttributes.OnDataAppended( + uint32_t(NS_ERROR_DOM_QUOTA_EXCEEDED_ERR)); } if (NS_WARN_IF(!mTrackBuffersManager)) { - return (*aErrorCb)(aErrorCbContext, uint32_t(NS_ERROR_NOT_INITIALIZED)); + return mCurrentAttributes.OnDataAppended( + uint32_t(NS_ERROR_NOT_INITIALIZED)); } - RefPtr self = this; mTrackBuffersManager->AppendData(data.forget(), mCurrentAttributes) ->Then(AbstractThread::MainThread(), __func__, - [self, this, aSuccessCb, aSuccessCbContext]( - const SourceBufferTask::AppendBufferResult& aResult) { - AppendDataCompletedWithSuccess( - aResult, aSuccessCb, aSuccessCbContext); - }, - [self, this, aErrorCb, aErrorCbContext](const MediaResult& aError) { - AppendDataErrored(aError, aErrorCb, aErrorCbContext); - }) + this, + &SourceBuffer::AppendDataCompletedWithSuccess, + &SourceBuffer::AppendDataErrored) ->Track(mPendingAppend); } void SourceBuffer::AppendDataCompletedWithSuccess( - const SourceBufferTask::AppendBufferResult& aResult, - success_callback_t aSuccessCb, - void* aSuccessCbContext) + const SourceBufferTask::AppendBufferResult& aResult) { MOZ_ASSERT(mCurrentAttributes.GetUpdating()); mPendingAppend.Complete(); @@ -120,14 +108,13 @@ SourceBuffer::AppendDataCompletedWithSuccess( if (!mCurrentAttributes.GetActive()) { mCurrentAttributes.SetActive(true); MSE_DEBUG("Init segment received"); - RefPtr self = this; mMediaSource->SourceBufferIsActive(this) ->Then(AbstractThread::MainThread(), __func__, - [self, this, aSuccessCb, aSuccessCbContext]() { + [this]() { MSE_DEBUG("Complete AppendBuffer operation"); mCompletionPromise.Complete(); - (*aSuccessCb)(aSuccessCbContext); + mCurrentAttributes.OnDataAppended(0 /* success */); }) ->Track(mCompletionPromise); } @@ -143,14 +130,12 @@ SourceBuffer::AppendDataCompletedWithSuccess( CheckEndTime(); if (!mCompletionPromise.Exists()) { - (*aSuccessCb)(aSuccessCbContext); + mCurrentAttributes.OnDataAppended(0 /* success */); } } void -SourceBuffer::AppendDataErrored(const MediaResult& aError, - error_callback_t aErrorCb, - void* aErrorCbContext) +SourceBuffer::AppendDataErrored(const MediaResult& aError) { MOZ_ASSERT(mCurrentAttributes.GetUpdating()); mPendingAppend.Complete(); @@ -162,7 +147,7 @@ SourceBuffer::AppendDataErrored(const MediaResult& aError, break; default: ResetParserState(); - (*aErrorCb)(aErrorCbContext, uint32_t(NS_ERROR_DOM_MEDIA_CANCELED)); + mCurrentAttributes.OnDataAppended(uint32_t(NS_ERROR_DOM_MEDIA_CANCELED)); break; } } @@ -202,21 +187,17 @@ SourceBuffer::ResetParserState() } void -SourceBuffer::RangeRemoval(double aStart, - double aEnd, - success_callback_t aSuccessCb, - void* aSuccessCbContext) +SourceBuffer::RangeRemoval(double aStart, double aEnd) { MOZ_ASSERT(NS_IsMainThread()); - RefPtr self = this; mTrackBuffersManager ->RangeRemoval(TimeUnit::FromSeconds(aStart), TimeUnit::FromSeconds(aEnd)) ->Then(AbstractThread::MainThread(), __func__, - [self, aSuccessCb, aSuccessCbContext](bool) { - self->mPendingRemoval.Complete(); - (*aSuccessCb)(aSuccessCbContext); + [this](bool) { + mPendingRemoval.Complete(); + mCurrentAttributes.OnRangeRemoved(); }, []() { MOZ_ASSERT(false); }) ->Track(mPendingRemoval); diff --git a/gecko-media/gecko/glue/include/GeckoMediaSourceBuffer.h b/gecko-media/gecko/glue/include/GeckoMediaSourceBuffer.h index 435bd1f..85ced0d 100644 --- a/gecko-media/gecko/glue/include/GeckoMediaSourceBuffer.h +++ b/gecko-media/gecko/glue/include/GeckoMediaSourceBuffer.h @@ -64,14 +64,13 @@ struct GeckoMediaSourceBufferImpl void (*mSetUpdating)(void*, bool); bool (*mGetActive)(void*); void (*mSetActive)(void*, bool); + void (*mOnDataAppended)(void*, uint32_t); + void (*mOnRangeRemoved)(void*); }; SourceBuffer* GetSourceBuffer(const size_t aId); -typedef void (*success_callback_t)(void*); -typedef void (*error_callback_t)(void*, uint32_t); - extern "C" { // TODO error handling (i.e. aId or aParentId may not be valid) void @@ -92,11 +91,7 @@ GeckoMedia_SourceBuffer_EvictData(size_t aId, void GeckoMedia_SourceBuffer_AppendData(size_t aId, const uint8_t* aData, - size_t aLength, - success_callback_t aSuccessCb, - void* aSuccessCbContext, - error_callback_t aErrorCb, - void* aErrorCbContext); + size_t aLength); void GeckoMedia_SourceBuffer_AbortBufferAppend(size_t aId); @@ -107,9 +102,7 @@ GeckoMedia_SourceBuffer_ResetParserState(size_t aId); void GeckoMedia_SourceBuffer_RangeRemoval(size_t aId, double aStart, - double aEnd, - success_callback_t aSuccessCb, - void* aSuccessCbContext); + double aEnd); } #endif // GeckoMediaSourceBuffer_h_ \ No newline at end of file diff --git a/gecko-media/gecko/glue/include/SourceBuffer.h b/gecko-media/gecko/glue/include/SourceBuffer.h index cc2f6f9..c892114 100644 --- a/gecko-media/gecko/glue/include/SourceBuffer.h +++ b/gecko-media/gecko/glue/include/SourceBuffer.h @@ -31,21 +31,13 @@ class SourceBuffer final void EvictData(size_t aLength, bool* aBufferFull); - void AppendData(const uint8_t* aData, - size_t aLength, - success_callback_t aSuccessCb, - void* aSuccessCbContext, - error_callback_t aErrorCb, - void* aErrorCbContext); + void AppendData(const uint8_t* aData, size_t aLength); void AbortBufferAppend(); void ResetParserState(); - void RangeRemoval(double aStart, - double aEnd, - success_callback_t aSuccessCb, - void* aSuccessCbContext); + void RangeRemoval(double aStart, double aEnd); private: friend class MediaSource; @@ -58,12 +50,8 @@ class SourceBuffer final void CheckEndTime(); void AppendDataCompletedWithSuccess( - const SourceBufferTask::AppendBufferResult& aResult, - success_callback_t aSuccessCb, - void* aSuccessCbContext); - void AppendDataErrored(const MediaResult& aError, - error_callback_t aErrorCb, - void* aErrorCbContext); + const SourceBufferTask::AppendBufferResult& aResult); + void AppendDataErrored(const MediaResult& aError); bool GetActive() { return mCurrentAttributes.GetActive(); } void SetActive(bool aActive) { mCurrentAttributes.SetActive(aActive); } @@ -75,7 +63,8 @@ class SourceBuffer final RefPtr mTrackBuffersManager; MozPromiseRequestHolder mPendingAppend; - MozPromiseRequestHolder mPendingRemoval; + MozPromiseRequestHolder + mPendingRemoval; MozPromiseRequestHolder mCompletionPromise; }; diff --git a/gecko-media/gecko/glue/include/SourceBufferAttributes.h b/gecko-media/gecko/glue/include/SourceBufferAttributes.h index 60cbcf8..b0d8de1 100644 --- a/gecko-media/gecko/glue/include/SourceBufferAttributes.h +++ b/gecko-media/gecko/glue/include/SourceBufferAttributes.h @@ -109,6 +109,18 @@ class SourceBufferAttributes aGroupEndTimestamp.ToSeconds()); } + void OnDataAppended(uint32_t aResult) + { + CALLBACK_GUARD_VOID(OnDataAppended); + (*mImpl.mOnDataAppended)(mImpl.mContext, aResult); + } + + void OnRangeRemoved() + { + CALLBACK_GUARD_VOID(OnRangeRemoved); + (*mImpl.mOnRangeRemoved)(mImpl.mContext); + } + // mGenerateTimestamp isn't mutable once the source buffer has been // constructed bool mGenerateTimestamps; diff --git a/gecko-media/src/lib.rs b/gecko-media/src/lib.rs index 141f5ee..231a834 100644 --- a/gecko-media/src/lib.rs +++ b/gecko-media/src/lib.rs @@ -427,11 +427,15 @@ mod tests { assert_eq!(gecko_media.is_type_supported("audio/wav"), false); } - struct SourceBufferAttributes {} + struct SourceBufferAttributes { + pub expect_data_appended_success: Cell, + } impl SourceBufferAttributes { pub fn new() -> Self { - SourceBufferAttributes {} + SourceBufferAttributes { + expect_data_appended_success: Cell::new(true), + } } } @@ -478,6 +482,14 @@ mod tests { false } fn set_active(&self, _: bool) {} + fn on_data_appended(&self, result: u32) { + if self.expect_data_appended_success.get() { + assert!(result == 0); + } else { + assert!(result != 0); + } + } + fn on_range_removed(&self) {} } struct SourceBufferDom { @@ -496,30 +508,24 @@ mod tests { } } - pub fn append_data(&self, data: *const u8, len: usize, success_cb: S, error_cb: E) - where - S: Fn(), - E: Fn(u32), - { - self.gecko_media.append_data( - data, - len, - success_cb, - error_cb, + pub fn append_data(&self, data: *const u8, len: usize, expect_success: bool) { + self.attributes.expect_data_appended_success.set( + expect_success, ); + self.gecko_media.append_data(data, len); } } fn test_source_buffer() { let media_source = MediaSourceDom::new(); - let source_buffer = SourceBufferDom::new(&media_source, "video/mp4"); + let source_buffer = Box::new(SourceBufferDom::new(&media_source, "video/mp4")); let empty: [u8; 0] = []; - // TODO For now this only tests that the mechanism to send closures through FFI works. // Should throw error because no decoder is attached yet. - source_buffer.append_data(empty.as_ptr(), empty.len(), || unreachable!(), |_| { - // TODO check error code. - assert!(true); - }); + source_buffer.append_data( + empty.as_ptr(), + empty.len(), + false, /* expect error: no decoder */ + ); } #[test] diff --git a/gecko-media/src/mse/sourcebuffer.rs b/gecko-media/src/mse/sourcebuffer.rs index ac8675b..e7b112f 100644 --- a/gecko-media/src/mse/sourcebuffer.rs +++ b/gecko-media/src/mse/sourcebuffer.rs @@ -47,24 +47,10 @@ impl SourceBuffer { receiver.recv().unwrap(); } - pub fn append_data(&self, data: *const u8, len: usize, success_cb: F, error_cb: E) - where - F: FnOnce(), - E: FnOnce(u32), - { + pub fn append_data(&self, data: *const u8, len: usize) { let id = self.id; - let success_cb = &success_cb as *const _ as *mut c_void; - let error_cb = &error_cb as *const _ as *mut c_void; self.gecko_media.queue_task(move || unsafe { - GeckoMedia_SourceBuffer_AppendData( - id, - data, - len, - Some(success_callback_wrapper::), - success_cb, - Some(error_callback_wrapper::), - error_cb, - ); + GeckoMedia_SourceBuffer_AppendData(id, data, len); }); } @@ -82,20 +68,10 @@ impl SourceBuffer { }); } - pub fn range_removal(&self, start: f64, end: f64, success_cb: F) - where - F: FnOnce(), - { + pub fn range_removal(&self, start: f64, end: f64) { let id = self.id; - let success_cb = &success_cb as *const _ as *mut c_void; self.gecko_media.queue_task(move || unsafe { - GeckoMedia_SourceBuffer_RangeRemoval( - id, - start, - end, - Some(success_callback_wrapper::), - success_cb, - ); + GeckoMedia_SourceBuffer_RangeRemoval(id, start, end); }); } } @@ -127,6 +103,8 @@ pub trait SourceBufferImpl { fn set_updating(&self, bool); fn get_active(&self) -> bool; fn set_active(&self, bool); + fn on_data_appended(&self, u32); + fn on_range_removed(&self); } pub fn to_ffi_callbacks(callbacks: Rc) -> GeckoMediaSourceBufferImpl { @@ -156,12 +134,18 @@ pub fn to_ffi_callbacks(callbacks: Rc) -> GeckoMediaSourceBuff impl_simple_ffi_setter_wrapper!(set_updating, bool); impl_simple_ffi_getter_wrapper!(get_active, bool); impl_simple_ffi_setter_wrapper!(set_active, bool); + impl_simple_ffi_callback_wrapper!(on_range_removed); unsafe extern "C" fn get_group_start_timestamp(ptr: *mut c_void, timestamp: *mut f64) { let wrapper = &*(ptr as *mut Wrapper); wrapper.callbacks.get_group_start_timestamp(timestamp); } + unsafe extern "C" fn on_data_appended(ptr: *mut c_void, result: u32) { + let wrapper = &*(ptr as *mut Wrapper); + wrapper.callbacks.on_data_appended(result); + } + GeckoMediaSourceBufferImpl { mContext: Box::into_raw(Box::new(Wrapper { callbacks, @@ -189,25 +173,7 @@ pub fn to_ffi_callbacks(callbacks: Rc) -> GeckoMediaSourceBuff mSetUpdating: Some(set_updating), mGetActive: Some(get_active), mSetActive: Some(set_active), - } -} - -extern "C" fn success_callback_wrapper(closure: *mut c_void) -where - F: FnOnce(), -{ - let opt_closure = closure as *mut Option; - unsafe { - (*opt_closure).take().unwrap()(); - } -} - -extern "C" fn error_callback_wrapper(closure: *mut c_void, error: u32) -where - F: FnOnce(u32), -{ - let opt_closure = closure as *mut Option; - unsafe { - (*opt_closure).take().unwrap()(error); + mOnDataAppended: Some(on_data_appended), + mOnRangeRemoved: Some(on_range_removed), } }