Closed Bug 1413116 Opened 3 years ago Closed 3 years ago

MediaRecorder should raise error when MutableBlobStorage::Append fails

Categories

(Core :: Audio/Video: Recording, enhancement, P2)

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

Attachments

(2 files)

Bug 1412894 makes MutableBlobStorage::Append return an error if it failed writing  to a file. We do currently not handle this error.
Rank: 18
Priority: -- → P2
Comment on attachment 8924468 [details]
Bug 1413116 - Handle errors from MutableBlobStorage::Append.

https://reviewboard.mozilla.org/r/195744/#review200878

::: dom/media/MediaRecorder.cpp:284
(Diff revision 1)
>        mSession->MaybeCreateMutableBlobStorage();
>        for (uint32_t i = 0; i < mBuffer.Length(); i++) {
>          if (!mBuffer[i].IsEmpty()) {
> -          mSession->mMutableBlobStorage->Append(mBuffer[i].Elements(),
> +          nsresult rv = mSession->mMutableBlobStorage->Append(mBuffer[i].Elements(),
> -                                                mBuffer[i].Length());
> +                                                              mBuffer[i].Length());
> +          if (NS_FAILED(rv)) {

NS_WARN_IF(NS_FAILED(rv))

::: dom/media/MediaRecorder.cpp:285
(Diff revision 1)
>        for (uint32_t i = 0; i < mBuffer.Length(); i++) {
>          if (!mBuffer[i].IsEmpty()) {
> -          mSession->mMutableBlobStorage->Append(mBuffer[i].Elements(),
> +          nsresult rv = mSession->mMutableBlobStorage->Append(mBuffer[i].Elements(),
> -                                                mBuffer[i].Length());
> +                                                              mBuffer[i].Length());
> +          if (NS_FAILED(rv)) {
> +            mSession->DoSessionEndTask(rv);

Is DoSessionEndTask able to deal with multiple calls?
Attachment #8924468 - Flags: review?(amarchesini) → review+
Comment on attachment 8924468 [details]
Bug 1413116 - Handle errors from MutableBlobStorage::Append.

https://reviewboard.mozilla.org/r/195744/#review200878

> Is DoSessionEndTask able to deal with multiple calls?

It is with the other patch. That said it probably makes more sense to break the loop there and drop remaining buffers.
Comment on attachment 8924468 [details]
Bug 1413116 - Handle errors from MutableBlobStorage::Append.

https://reviewboard.mozilla.org/r/195744/#review201942
Attachment #8924468 - Flags: review?(bvandyk) → review+
Comment on attachment 8924469 [details]
Bug 1413116 - Consolidate MediaRecorder::Session state and avoid "start" and "error" races.

https://reviewboard.mozilla.org/r/195746/#review201950

::: dom/media/MediaRecorder.cpp:1202
(Diff revision 1)
> -  // Indicate the session had fire start event. Encoding thread only.
> +  // or Stop().
> -  bool mIsStartEventFired;
> -  // False if the InitEncoder called successfully, ensure the
> -  // ExtractRunnable/DestroyRunnable will end the session.
>    // Main thread only.
> -  bool mNeedSessionEndTask;
> +  Result<RunningState, nsresult> mRunningState;

What do you think about using an enum to track stopped? Seeing ERR(NS_OK) threw me a little when I first read it.
Comment on attachment 8924469 [details]
Bug 1413116 - Consolidate MediaRecorder::Session state and avoid "start" and "error" races.

https://reviewboard.mozilla.org/r/195746/#review203366
Attachment #8924469 - Flags: review?(bvandyk) → review+
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/aec9d75a9a1b
Handle errors from MutableBlobStorage::Append. r=baku,SingingTree
https://hg.mozilla.org/integration/autoland/rev/1935e4699d80
Consolidate MediaRecorder::Session state and avoid "start" and "error" races. r=SingingTree
https://hg.mozilla.org/mozilla-central/rev/aec9d75a9a1b
https://hg.mozilla.org/mozilla-central/rev/1935e4699d80
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.