Jean-Yves Avenard [:jya] Assignee
	Description • 8 years ago

It is impossible to tell at this stage on what caused a MEDIA_DECODE_ERROR.

Aim is to provide further details; 

It was suggested that we could extend the current media element error attribute.

Passing error such as demuxer, why initialisation failed, OOM etc... would be a valuable diagnostic tool

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)
	Comment 1 • 8 years ago

This is a P1 because we need to know what proportion of playback errors are caused by OOM.

	JW Wang [:jwwang] [:jw_wang]
	Comment 30 • 8 years ago
mozreview-review

Comment on attachment 8790168 [details]
Bug 1299072: P3. Add MediaResult object.

https://reviewboard.mozilla.org/r/78070/#review76540

::: dom/media/MediaResult.h:25
(Diff revision 2)
> +{
> +public:
> +  MOZ_IMPLICIT MediaResult(nsresult aResult)
> +  : mCode(aResult)
> +  { }
> +  explicit MediaResult(nsresult aResult, const nsACString& aMessage)

You don't need 'explicit' when it takes 2 parameters.

::: dom/media/MediaResult.h:33
(Diff revision 2)
> +  { }
> +  MediaResult(nsresult aResult, const char* aMessage)
> +  : mCode(aResult)
> +  , mMessage(aMessage)
> +  { }
> +  MediaResult() : mCode(NS_OK) { }

The default constructor might cause surprise if the client code forgets to supply an error code to the constructor.

Explicit is better than implicit.

	Jean-Yves Avenard [:jya] Assignee
	Comment 31 • 8 years ago
mozreview-review-reply

Comment on attachment 8790168 [details]
Bug 1299072: P3. Add MediaResult object.

https://reviewboard.mozilla.org/r/78070/#review76540

> The default constructor might cause surprise if the client code forgets to supply an error code to the constructor.
> 
> Explicit is better than implicit.

No..

To me a default constructed MediaResult is the same as using NS_OK

	JW Wang [:jwwang] [:jw_wang]
	Comment 32 • 8 years ago
mozreview-review

Comment on attachment 8790170 [details]
Bug 1299072: P5. Don't rely on specific error to assess recoverability.

https://reviewboard.mozilla.org/r/78074/#review76544

	Jean-Yves Avenard [:jya] Assignee
	Comment 33 • 8 years ago
mozreview-review-reply

Comment on attachment 8790168 [details]
Bug 1299072: P3. Add MediaResult object.

https://reviewboard.mozilla.org/r/78070/#review76540

> You don't need 'explicit' when it takes 2 parameters.

it's a remnant of having the constructor takes an optional message.

> No..
> 
> To me a default constructed MediaResult is the same as using NS_OK

BTW, making the constructor MOZ_IMPLICIT is required if we want nsresult and MediaResult to be used interchangeably. 
Otherwise you'd have to always explicitly construct the object such as:
promise.Reject(MediaResult(NS_OK), __func__)
rather that the much simpler:
promise.Reject(NS_OK, __func__);

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 34 • 8 years ago
mozreview-review

Comment on attachment 8790166 [details]
Bug 1299072: P1. Add message attribute to MediaError object,

https://reviewboard.mozilla.org/r/78066/#review76548

::: dom/html/MediaError.cpp:38
(Diff revision 2)
> +NS_IMETHODIMP MediaError::GetMessage(nsAString& aResult)
> +{
> +  return NS_OK;

I was wondering if there was some danger with not changing the out-param string but still returning NS_OK, but I see you do overwrite it in P10, well played!

	JW Wang [:jwwang] [:jw_wang]
	Comment 35 • 8 years ago
mozreview-review

Comment on attachment 8790171 [details]
Bug 1299072: P6. Pass decoding error details to MDSM and relatives.

https://reviewboard.mozilla.org/r/78076/#review76550

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 36 • 8 years ago
mozreview-review

Comment on attachment 8790167 [details]
Bug 1299072: P2. Add media decoding error types.

https://reviewboard.mozilla.org/r/78068/#review76552

r+ with my concern addressed -- as I thought OOMs were the first thing we wanted to expose, see bug 1299072 comment 1.

::: xpcom/base/ErrorList.h:980
(Diff revision 2)
>    /* HTMLMediaElement API errors from https://html.spec.whatwg.org/multipage/embedded-content.html#media-elements */
>    ERROR(NS_ERROR_DOM_MEDIA_ABORT_ERR,           FAILURE(1)),
>    ERROR(NS_ERROR_DOM_MEDIA_NOT_ALLOWED_ERR,     FAILURE(2)),
>    ERROR(NS_ERROR_DOM_MEDIA_NOT_SUPPORTED_ERR,   FAILURE(3)),
> +
> +  /* HTMLMediaElement internal decoding error */

Don't we want an "OOM" error as well?

	JW Wang [:jwwang] [:jw_wang]
	Comment 37 • 8 years ago
mozreview-review

Comment on attachment 8790172 [details]
Bug 1299072: P7. Use MediaResult with MetadataPromise.

https://reviewboard.mozilla.org/r/78078/#review76554

	JW Wang [:jwwang] [:jw_wang]
	Comment 38 • 8 years ago
mozreview-review

Comment on attachment 8790173 [details]
Bug 1299072: P8. Pass decoding error details through SeekTask.

https://reviewboard.mozilla.org/r/78080/#review76556

	Jean-Yves Avenard [:jya] Assignee
	Comment 39 • 8 years ago
mozreview-review-reply

Comment on attachment 8790167 [details]
Bug 1299072: P2. Add media decoding error types.

https://reviewboard.mozilla.org/r/78068/#review76552

> Don't we want an "OOM" error as well?

There's already NS_ERROR_OUT_OF_MEMORY

	JW Wang [:jwwang] [:jw_wang]
	Comment 40 • 8 years ago
mozreview-review

Comment on attachment 8790174 [details]
Bug 1299072: P9. Pass decoding error details to MediaDecoder.

https://reviewboard.mozilla.org/r/78082/#review76560

::: dom/media/MediaDecoder.h:594
(Diff revision 2)
>                        MediaDecoderEventVisibility aEventVisibility);
>  
>    MediaEventSource<void>*
>    DataArrivedEvent() override { return &mDataArrivedEvent; }
>  
> -  void OnPlaybackEvent(MediaEventType aEvent);
> +  void OnPlaybackEvent(MediaEventType aEvent, const MediaResult& aError);

It doesn't scale when we need to pass additional data along with playback events.

For ex, we want to pass a TimeStamp to indicate the time when seeking begins. We will have to do:

mOnPlaybackEvent.Notify(MediaEventType::SeekStarted, NS_OK, TimeStamp(...));

However, the error code is not used by other playback events at all.

Please add a new member dedicated to publish error events like: MediaEventProducer<MediaResult> mOnPlaybackError;

	JW Wang [:jwwang] [:jw_wang]
	Comment 41 • 8 years ago
mozreview-review

Comment on attachment 8790175 [details]
Bug 1299072: P10. Pass decoding error details to media element's error attribute.

https://reviewboard.mozilla.org/r/78084/#review76564

	JW Wang [:jwwang] [:jw_wang]
	Comment 42 • 8 years ago
mozreview-review-reply

Comment on attachment 8790168 [details]
Bug 1299072: P3. Add MediaResult object.

https://reviewboard.mozilla.org/r/78070/#review76540

> BTW, making the constructor MOZ_IMPLICIT is required if we want nsresult and MediaResult to be used interchangeably. 
> Otherwise you'd have to always explicitly construct the object such as:
> promise.Reject(MediaResult(NS_OK), __func__)
> rather that the much simpler:
> promise.Reject(NS_OK, __func__);

This comment should apply to the |MOZ_IMPLICIT MediaResult(nsresult aResult)| construtor. It has nothing to do with the default constructor.

	Jean-Yves Avenard [:jya] Assignee
	Comment 43 • 8 years ago
mozreview-review-reply

Comment on attachment 8790174 [details]
Bug 1299072: P9. Pass decoding error details to MediaDecoder.

https://reviewboard.mozilla.org/r/78082/#review76560

> It doesn't scale when we need to pass additional data along with playback events.
> 
> For ex, we want to pass a TimeStamp to indicate the time when seeking begins. We will have to do:
> 
> mOnPlaybackEvent.Notify(MediaEventType::SeekStarted, NS_OK, TimeStamp(...));
> 
> However, the error code is not used by other playback events at all.
> 
> Please add a new member dedicated to publish error events like: MediaEventProducer<MediaResult> mOnPlaybackError;

But we do not pass TimeStamp... so why is this an issue?

I had considered the issue you mentioned, and decided against it for the following reasons:

1- You have OnPlaybackErrorEvent and continue to use OnPlaybackEvent
  -> Consequences: you have two tasks being fired serially, on to handle that we got an error, and the next one to actually provide the error. So now you have to handle differently, buffering the error, or not handle the error at the time the playback event

2- You now have OnPlaybackErrorEvent and stop using OnPlaybackEvent to go into media error.
  -> Consequences: that's even worse because while right now there's only one listener, should in the future we get a new listener. The new code would have to be aware that the handling of the error playback event is handled differently to all the others.


So in any case, I disagree that using a new event producer makes things simpler (it doesn't) or more extensible. If future change require to pass different type of events, then using a Variant instead is the way to go. But this shouldn't be a concern today.

I hope you'll agree with my assessment. I didn't choose the way I went lightly, every aspects were carefully considered.

	Jean-Yves Avenard [:jya] Assignee
	Comment 44 • 8 years ago
mozreview-review-reply

Comment on attachment 8790168 [details]
Bug 1299072: P3. Add MediaResult object.

https://reviewboard.mozilla.org/r/78070/#review76540

> This comment should apply to the |MOZ_IMPLICIT MediaResult(nsresult aResult)| construtor. It has nothing to do with the default constructor.

you pointed out two things in your comment:
1- Default constructor (which is my 1st answer)
2- That explicit is better than implicit (which is my 2nd answer)

two comments, two answers :)

	JW Wang [:jwwang] [:jw_wang]
	Comment 45 • 8 years ago
mozreview-review-reply

Comment on attachment 8790168 [details]
Bug 1299072: P3. Add MediaResult object.

https://reviewboard.mozilla.org/r/78070/#review76540

> you pointed out two things in your comment:
> 1- Default constructor (which is my 1st answer)
> 2- That explicit is better than implicit (which is my 2nd answer)
> 
> two comments, two answers :)

|MediaResult result;| is equivalent to |nsresult result = NS_OK;| but less obvious without looking at the implementation. That's why I said 'Explicit is better than implicit.'.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 46 • 8 years ago
mozreview-review

Comment on attachment 8790168 [details]
Bug 1299072: P3. Add MediaResult object.

https://reviewboard.mozilla.org/r/78070/#review76562

r+ with nits:

::: dom/media/MediaResult.h:7
(Diff revision 2)
> +/* vim:set ts=2 sw=2 sts=2 et cindent: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#if !defined(MediaResult_h_)

Style: Include guard must have the exact form "#ifndef <guard>".
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

::: dom/media/MediaResult.h:23
(Diff revision 2)
> +
> +class MediaResult
> +{
> +public:
> +  MOZ_IMPLICIT MediaResult(nsresult aResult)
> +  : mCode(aResult)

Style: Indent initializers.
(Same with other constructors below.)

::: dom/media/MediaResult.h:33
(Diff revision 2)
> +  { }
> +  MediaResult(nsresult aResult, const char* aMessage)
> +  : mCode(aResult)
> +  , mMessage(aMessage)
> +  { }
> +  MediaResult() : mCode(NS_OK) { }

I'm with JW here: Remove the empty constructor. For clarity, users should explicitly call MediaResult(NS_OK), or just the traditional NS_OK which your implicit constructor allows.

::: dom/media/MediaResult.h:33
(Diff revision 2)
> +  MediaResult() : mCode(NS_OK) { }
> +

Also, you could define move constructor&assignment as '= default', which would allow cheap returns from functions and their subsequent storage in the caller.

::: dom/media/MediaResult.h:39
(Diff revision 2)
> +  bool operator==(nsresult aResult) const { return aResult == mCode; }
> +  bool operator!=(nsresult aResult) const { return aResult != mCode; }

You may want to also define global comparator functions that take (nsresult, const MediaResult&), to allow nsresult on the left.

	JW Wang [:jwwang] [:jw_wang]
	Comment 47 • 8 years ago
mozreview-review

Comment on attachment 8790174 [details]
Bug 1299072: P9. Pass decoding error details to MediaDecoder.

https://reviewboard.mozilla.org/r/78082/#review76574

::: dom/media/MediaDecoder.h:594
(Diff revision 2)
>                        MediaDecoderEventVisibility aEventVisibility);
>  
>    MediaEventSource<void>*
>    DataArrivedEvent() override { return &mDataArrivedEvent; }
>  
> -  void OnPlaybackEvent(MediaEventType aEvent);
> +  void OnPlaybackEvent(MediaEventType aEvent, const MediaResult& aError);

1. I mean OnPlaybackErrorEvent and OnPlaybackEvent will be used exclusively which also means to remove the field of MediaEventType::DecodeError because OnPlaybackErrorEvent will take its role.

2. The new code will certainly be aware of the change (a compile error :)) since we remove a field from MediaEventType.

	Jean-Yves Avenard [:jya] Assignee
	Comment 48 • 8 years ago
mozreview-review-reply

Comment on attachment 8790168 [details]
Bug 1299072: P3. Add MediaResult object.

https://reviewboard.mozilla.org/r/78070/#review76562

> I'm with JW here: Remove the empty constructor. For clarity, users should explicitly call MediaResult(NS_OK), or just the traditional NS_OK which your implicit constructor allows.

Actually the primary reason for me to create one os that one of the container class (maybe thats Variant) requires a default constructor

	Jean-Yves Avenard [:jya] Assignee
	Comment 49 • 8 years ago
mozreview-review-reply

Comment on attachment 8790168 [details]
Bug 1299072: P3. Add MediaResult object.

https://reviewboard.mozilla.org/r/78070/#review76562

> Also, you could define move constructor&assignment as '= default', which would allow cheap returns from functions and their subsequent storage in the caller.

Move constructors are automatically generated and creating one explicitly that only move the arguments would bring no benefit

	Jean-Yves Avenard [:jya] Assignee
	Comment 50 • 8 years ago
mozreview-review-reply

Comment on attachment 8790168 [details]
Bug 1299072: P3. Add MediaResult object.

https://reviewboard.mozilla.org/r/78070/#review76562

> You may want to also define global comparator functions that take (nsresult, const MediaResult&), to allow nsresult on the left.

The automatic conversion from MediaResult to nsresult makes it unecessary and is used thoroughly accross all other patches

	JW Wang [:jwwang] [:jw_wang]
	Comment 51 • 8 years ago
mozreview-review

Comment on attachment 8790174 [details]
Bug 1299072: P9. Pass decoding error details to MediaDecoder.

https://reviewboard.mozilla.org/r/78082/#review76590

Per discussion, I can accept this approach. We can come back at it for a better solution when we have more event data to pass for various playback events.

	Jean-Yves Avenard [:jya] Assignee
	Comment 52 • 8 years ago
mozreview-review-reply

Comment on attachment 8790168 [details]
Bug 1299072: P3. Add MediaResult object.

https://reviewboard.mozilla.org/r/78070/#review76562

> Actually the primary reason for me to create one os that one of the container class (maybe thats Variant) requires a default constructor

ok.. I'll remove it and make changes to all the follow up patches relying on having a default constructor

	Jean-Yves Avenard [:jya] Assignee
	Comment 53 • 8 years ago
mozreview-review-reply

Comment on attachment 8790174 [details]
Bug 1299072: P9. Pass decoding error details to MediaDecoder.

https://reviewboard.mozilla.org/r/78082/#review76590

As you mentioned we already have different media event for handling metadata loaded and first frame loaded. having a new event listener for handling error is okay too.

i don't want to leave code that you feel isn't optimal.. so I'll correct it as first requested.
we can switch to variant later all at once (including metadataloaded and firstframeloaded)

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 66 • 8 years ago
mozreview-review-reply

Comment on attachment 8790168 [details]
Bug 1299072: P3. Add MediaResult object.

https://reviewboard.mozilla.org/r/78070/#review76562

> Move constructors are automatically generated and creating one explicitly that only move the arguments would bring no benefit

Oops you're right, you didn't write any of copy/move/destructor special members, so they should all be default-generated.

> ok.. I'll remove it and make changes to all the follow up patches relying on having a default constructor

It would make sense to keep it if used in some containers, you're right. But if you manage to work around that, I think it would be worth it, thank you for trying.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 67 • 8 years ago
mozreview-review

Comment on attachment 8790169 [details]
Bug 1299072: P4. Return extended failure details to reader.

https://reviewboard.mozilla.org/r/78072/#review76604

r+ with nits:

::: dom/media/MediaFormatReader.h:331
(Diff revision 3)
>      }
>  
>      uint32_t mNumOfConsecutiveError;
>      uint32_t mMaxConsecutiveError;
>  
> -    Maybe<MediaDataDecoderError> mError;
> +    Maybe<MediaResult> mError;

Maybe for another patch, but now that we have this much more verbose MediaResult that includes an obvious non-error value (NS_OK), couldn't we get rid of the Maybe wrapper?

::: dom/media/MediaFormatReader.h:332
(Diff revision 3)
>      bool HasFatalError() const
>      {
> -      return mError.isSome() && mError.ref() == MediaDataDecoderError::FATAL_ERROR;
> +      return mError.isSome() && mError.ref() != NS_ERROR_DOM_MEDIA_DECODE_ERR;

Just to be sure: Anything other than DECODE is a fatal error?

::: dom/media/MediaFormatReader.cpp:1256
(Diff revision 3)
>      DrainDecoder(aTrack);
>      return;
>    }
>  
>    if (decoder.mError &&
> -      decoder.mError.ref() == MediaDataDecoderError::DECODE_ERROR) {
> +      decoder.mError.ref().Code() == NS_ERROR_DOM_MEDIA_DECODE_ERR) {

You wrote nsresult comparators, why not use them? :-)

::: dom/media/platforms/agnostic/BlankDecoderModule.cpp:83
(Diff revision 3)
>  private:
>    void OutputFrame(MediaData* aData)
>    {
>      if (!aData) {
> -      mCallback->Error(MediaDataDecoderError::FATAL_ERROR);
> +      mCallback->Error(MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,
> +                                        __func__));

Style: Left-align '__func__' with 'NS_...' above.
Same all over the patch, go down the mozreview patch view to find them all. (Or let me know if you'd prefer an error for each of them.)

::: dom/media/platforms/agnostic/TheoraDecoder.cpp:202
(Diff revision 3)
>    MOZ_ASSERT(mTaskQueue->IsCurrentThreadIn());
>    if (mIsFlushing) {
>      return;
>    }
>    if (DoDecode(aSample) == -1) {
> -    mCallback->Error(MediaDataDecoderError::DECODE_ERROR);
> +    mCallback->Error(MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,

DECODE has become FATAL, are you sure?

::: dom/media/platforms/wrappers/FuzzingWrapper.cpp:179
(Diff revision 3)
> -                                               &DecoderCallbackFuzzingWrapper::Error,
> +                                          &DecoderCallbackFuzzingWrapper::Error,
> -                                               aError));
> +                                          aError));

Style: Alignment.

::: dom/media/platforms/wrappers/H264Converter.cpp:92
(Diff revision 3)
>      }
>    } else {
>      rv = CheckForSPSChange(aSample);
>    }
>    if (NS_FAILED(rv)) {
> -    mCallback->Error(MediaDataDecoderError::DECODE_ERROR);
> +    mCallback->Error(MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__));

DECODE has become FATAL, are you sure?

::: dom/media/platforms/wrappers/H264Converter.cpp:103
(Diff revision 3)
>      return;
>    }
>  
>    if (!mNeedAVCC &&
>        !mp4_demuxer::AnnexB::ConvertSampleToAnnexB(aSample)) {
> -    mCallback->Error(MediaDataDecoderError::FATAL_ERROR);
> +    mCallback->Error(MediaResult(NS_ERROR_DOM_MEDIA_DECODE_ERR, __func__));

FATAL has become DECODE, are you sure?

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 68 • 8 years ago
mozreview-review

Comment on attachment 8790176 [details]
Bug 1299072: P11. Use MediaResult with AppendBuffer promises.

https://reviewboard.mozilla.org/r/78086/#review76608

r+ with only one nit:

::: dom/media/mediasource/SourceBuffer.h:158
(Diff revision 3)
>  
>    // Implement the "Append Error Algorithm".
>    // Will call endOfStream() with "decode" error if aDecodeError is true.
>    // 3.5.3 Append Error Algorithm
>    // http://w3c.github.io/media-source/#sourcebuffer-append-error
> -  void AppendError(bool aDecoderError);
> +  void AppendError(const MediaResult& aDecodeError);

You have renamed the parameter 'aError' in the cpp, I think you should do the same here for consistency.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 69 • 8 years ago
mozreview-review

Comment on attachment 8790177 [details]
Bug 1299072: P12. Use MediaResult for MediaDataDemuxer promises.

https://reviewboard.mozilla.org/r/78088/#review76610

r+ with nits:

::: dom/media/ADTSDemuxer.cpp:518
(Diff revision 3)
> -  if (!aNumSamples) {
> +  MOZ_ASSERT(aNumSamples);
> -    return SamplesPromise::CreateAndReject(
> -      DemuxerFailureReason::DEMUXER_ERROR, __func__);
> -  }

This seems unrelated to the bug at hand; why the change from a test&promise-rejection to an assert?
(Same in WaveDemuxer.cpp and WebMDemuxer.cpp)

::: dom/media/MediaFormatReader.cpp:1565
(Diff revision 3)
>    MOZ_ASSERT(OnTaskQueue());
>    LOG("Skipping failed, skipped %u frames", aFailure.mSkipped);
>    mSkipRequest.Complete();
>  
> -  switch (aFailure.mFailure) {
> -    case DemuxerFailureReason::END_OF_STREAM: MOZ_FALLTHROUGH;
> +  switch (aFailure.mFailure.Code()) {
> +    case NS_ERROR_DOM_MEDIA_END_OF_STREAM: MOZ_FALLTHROUGH;

No need for MOZ_FALLTHROUGH here, I think you could just remove it as part of this patch.

::: dom/media/mediasource/TrackBuffersManager.cpp:1166
(Diff revision 3)
> -    case DemuxerFailureReason::DEMUXER_ERROR:
> -      RejectProcessing(NS_ERROR_FAILURE, __func__);
> +    case NS_ERROR_DOM_MEDIA_CANCELED:
> +      RejectProcessing(aError, __func__);

Looks exactly like the default case, so you could remove this one.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 70 • 8 years ago
mozreview-review

Comment on attachment 8790178 [details]
Bug 1299072: P13. Use MediaResult with TrackBuffersManager internal promises.

https://reviewboard.mozilla.org/r/78112/#review76612

r+ with suggestion:

::: dom/media/mediasource/TrackBuffersManager.cpp:748
(Diff revision 3)
>  }
>  
>  void
> -TrackBuffersManager::RejectAppend(nsresult aRejectValue, const char* aName)
> +TrackBuffersManager::RejectAppend(const MediaResult& aRejectValue, const char* aName)
>  {
> -  MSE_DEBUG("rv=%d", aRejectValue);
> +  MSE_DEBUG("rv=%u", aRejectValue.Code());

Would it be worth logging the fuller aRejectValue.Description()?
If yes, previous patches also had similar code that you may want to update.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 71 • 8 years ago
mozreview-review

Comment on attachment 8790179 [details]
Bug 1299072: P14. Use MediaResult between TrackBuffersManager and MediaSourceDemuxer.

https://reviewboard.mozilla.org/r/78114/#review76614

	Jean-Yves Avenard [:jya] Assignee
	Comment 72 • 8 years ago
mozreview-review-reply

Comment on attachment 8790169 [details]
Bug 1299072: P4. Return extended failure details to reader.

https://reviewboard.mozilla.org/r/78072/#review76604

> Maybe for another patch, but now that we have this much more verbose MediaResult that includes an obvious non-error value (NS_OK), couldn't we get rid of the Maybe wrapper?

I could do so in another patch "maybe" (pun intended).
Could use the value of NS_OK as special value.

> Just to be sure: Anything other than DECODE is a fatal error?

at this stage, I believe so.

The MediaDataDecoder were all checked so that any values returned will either be DECODE_ERR if it's okay to try the next keyframe once again, or not bother trying again (fatal)

Those are: OOM, invalid metadata (it will never become valid again), overflow, invalid duration, or conditions where the stream is so faulty that we can't even create the decoder (that includes no valid SPS found).

> DECODE has become FATAL, are you sure?

no :)
will fix that...

> DECODE has become FATAL, are you sure?

Yes... Not being able to convert an annex B only mean one thing at this stage: OOM, or the metadata's extradata was invalid. Neither of which we can recover from.

	Jean-Yves Avenard [:jya] Assignee
	Comment 73 • 8 years ago
mozreview-review-reply

Comment on attachment 8790177 [details]
Bug 1299072: P12. Use MediaResult for MediaDataDemuxer promises.

https://reviewboard.mozilla.org/r/78088/#review76610

> This seems unrelated to the bug at hand; why the change from a test&promise-rejection to an assert?
> (Same in WaveDemuxer.cpp and WebMDemuxer.cpp)

because the error returned doesn't make sense, it's not a demuxer error; it's the caller error.

The only user of the demuxers is the MediaFormatReader or TrackBufferManagers, it always call GetSamples with a value of 1 or -1 (all of them)

> No need for MOZ_FALLTHROUGH here, I think you could just remove it as part of this patch.

why not?
I thought all cases falling through required it... It was there before, I also don't believe I was the one who added it in the first place.

> Looks exactly like the default case, so you could remove this one.

Yes, I had thought the same at the time...

will do

	Jean-Yves Avenard [:jya] Assignee
	Comment 74 • 8 years ago
mozreview-review-reply

Comment on attachment 8790178 [details]
Bug 1299072: P13. Use MediaResult with TrackBuffersManager internal promises.

https://reviewboard.mozilla.org/r/78112/#review76612

> Would it be worth logging the fuller aRejectValue.Description()?
> If yes, previous patches also had similar code that you may want to update.

at this stage not really.

no description is ever provided by any of the demuxers.... So the description will be identical to the code converted to a string.

I've opened a bug to have the rust moov parser provide failure details...

	Jean-Yves Avenard [:jya] Assignee
	Comment 75 • 8 years ago
mozreview-review-reply

Comment on attachment 8790169 [details]
Bug 1299072: P4. Return extended failure details to reader.

https://reviewboard.mozilla.org/r/78072/#review76604

> You wrote nsresult comparators, why not use them? :-)

it's removed in P5

	Jean-Yves Avenard [:jya] Assignee
	Comment 76 • 8 years ago
mozreview-review-reply

Comment on attachment 8790176 [details]
Bug 1299072: P11. Use MediaResult with AppendBuffer promises.

https://reviewboard.mozilla.org/r/78086/#review76608

> You have renamed the parameter 'aError' in the cpp, I think you should do the same here for consistency.

I will keep aDecodeError as that's the name used in the spec. I did open a bug however that this parameter should be dropped as it's no longer used anymore. 
https://github.com/w3c/media-source/issues/159

	(no longer active)
	Comment 91 • 8 years ago
mozreview-review

Comment on attachment 8790166 [details]
Bug 1299072: P1. Add message attribute to MediaError object,

https://reviewboard.mozilla.org/r/78066/#review76800

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 108 • 8 years ago
mozreview-review

Comment on attachment 8790512 [details]
Bug 1299072: P15. Provide additional error details for most remaining decoders.

https://reviewboard.mozilla.org/r/78288/#review76818

r+ with a suggestion and minor concerns:

::: dom/media/platforms/agnostic/VPXDecoder.cpp:117
(Diff revision 1)
>    if (vpx_codec_err_t r = vpx_codec_decode(&mVPX, aSample->Data(), aSample->Size(), nullptr, 0)) {
>      LOG("VPX Decode error: %s", vpx_codec_err_to_string(r));
> -    return -1;
> +    return NS_ERROR_DOM_MEDIA_DECODE_ERR;

vpx_codec_err_t contains some useful values like 'VPX_CODEC_MEM_ERROR' (which we are most interested about), so it could be worth handling that value, and maybe others as you see fit.

::: dom/media/platforms/apple/AppleVTDecoder.cpp:449
(Diff revision 1)
>      mCallback->Error(MediaResult(NS_ERROR_OUT_OF_MEMORY, __func__));
> -    return NS_ERROR_OUT_OF_MEMORY;
> +    return MediaResult(NS_ERROR_OUT_OF_MEMORY, __func__);

A general concern:
I've seen this duplication (send an error to a callback, AND return the same/similar error) in other places.
In the best case, either/both will be put in the error object, so it should be fine.
But would there be a risk that we could be sending double events/notifications/etc.?

::: dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:334
(Diff revision 1)
>  FFmpegVideoDecoder<LIBAV_VER>::ProcessDrain()
>  {
>    RefPtr<MediaRawData> empty(new MediaRawData());
>    empty->mTimecode = mLastInputDts;
> -  while (DoDecode(empty) == DecodeResult::DECODE_FRAME) {
> -  }
> +  bool gotFrame = false;
> +  while (NS_SUCCEEDED(DoDecode(empty, &gotFrame)) && gotFrame);

You're not setting 'gotFrame' back to false between loops... Are you sure it's ok? (I only saw one spot where you do *aGotFrame = false)

And in case it is ok: Then do you really need to initialize gotFrame to false?

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 109 • 8 years ago
mozreview-review

Comment on attachment 8790513 [details]
Bug 1299072: P16. Add mochitest.

https://reviewboard.mozilla.org/r/78290/#review76824

	Boris Zbarsky [:bzbarsky]
	Comment 110 • 8 years ago
mozreview-review

Comment on attachment 8790175 [details]
Bug 1299072: P10. Pass decoding error details to media element's error attribute.

https://reviewboard.mozilla.org/r/78084/#review76826

::: dom/html/MediaError.cpp:42
(Diff revision 5)
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP MediaError::GetMessage(nsAString& aResult)
>  {
> +  aResult = NS_ConvertUTF8toUTF16(mMessage);

`CopyUTF8toUTF16(mMessage, aResult)` will do a bit less string allocation and copying.

	Boris Zbarsky [:bzbarsky]
	Comment 111 • 8 years ago
mozreview-review

Comment on attachment 8790166 [details]
Bug 1299072: P1. Add message attribute to MediaError object,

https://reviewboard.mozilla.org/r/78066/#review76830

::: dom/interfaces/html/nsIDOMMediaError.idl:28
(Diff revision 4)
>    /* No suitable media resource could be found */
>    const unsigned short MEDIA_ERR_SRC_NOT_SUPPORTED = 4;
>  
>    readonly attribute unsigned short code;
> +
> +  readonly attribute DOMString message;

I'm 98% sure nsIDOMMediaError is all dead code and can be removed (and the constants' use from C++ migrated to use the WebIDL versions instead).

Separate bug is probably best for this, but please do file one.

::: dom/webidl/MediaError.webidl:22
(Diff revision 4)
>    const unsigned short MEDIA_ERR_DECODE = 3;
>    const unsigned short MEDIA_ERR_SRC_NOT_SUPPORTED = 4;
>  
>    [Constant]
>    readonly attribute unsigned short code;
> +  readonly attribute DOMString message;

This attribute isn't in the spec.  Pending spec proposals and such, please put it behind a pref that's ifdefed in all.js to enable the attribut on non-release (nightly and aurora) channels only.

Once there is a spec proposal, please send an intent to ship and whatnot using the template at https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Intent_to_Ship and at that point we can consider flipping the pref on beta/release (and backporting that if this patch has already gotten to beta).

	Jean-Yves Avenard [:jya] Assignee
	Comment 112 • 8 years ago
mozreview-review-reply

Comment on attachment 8790512 [details]
Bug 1299072: P15. Provide additional error details for most remaining decoders.

https://reviewboard.mozilla.org/r/78288/#review76818

> vpx_codec_err_t contains some useful values like 'VPX_CODEC_MEM_ERROR' (which we are most interested about), so it could be worth handling that value, and maybe others as you see fit.

VPX is no longer used... I'll open a follow up bug on this were we can decide if we care or not

> A general concern:
> I've seen this duplication (send an error to a callback, AND return the same/similar error) in other places.
> In the best case, either/both will be put in the error object, so it should be fine.
> But would there be a risk that we could be sending double events/notifications/etc.?

the return value is no longer used.. this hsould be cleaned up. I'll open a side bug

> You're not setting 'gotFrame' back to false between loops... Are you sure it's ok? (I only saw one spot where you do *aGotFrame = false)
> 
> And in case it is ok: Then do you really need to initialize gotFrame to false?

gotFrame is initialised when it matters to false/true.
However, gotFrame isn't always set by DoDecode (like if we have a decoding error). To avoid CID errors about a variable potentially used not initialized, I set it to false.

though I agree, it serves no purpose.

	Jean-Yves Avenard [:jya] Assignee
	Comment 113 • 8 years ago
mozreview-review-reply

Comment on attachment 8790166 [details]
Bug 1299072: P1. Add message attribute to MediaError object,

https://reviewboard.mozilla.org/r/78066/#review76830

> I'm 98% sure nsIDOMMediaError is all dead code and can be removed (and the constants' use from C++ migrated to use the WebIDL versions instead).
> 
> Separate bug is probably best for this, but please do file one.

I opened bug 1302304

> This attribute isn't in the spec.  Pending spec proposals and such, please put it behind a pref that's ifdefed in all.js to enable the attribut on non-release (nightly and aurora) channels only.
> 
> Once there is a spec proposal, please send an intent to ship and whatnot using the template at https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Intent_to_Ship and at that point we can consider flipping the pref on beta/release (and backporting that if this patch has already gotten to beta).

As discussed on IRC, will make the attribute behind a spec, that is only set to true on aurora/central.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 133 • 8 years ago
mozreview-review

Comment on attachment 8790551 [details]
Bug 1299072: P17. Return last decoding error once threshold reached.

https://reviewboard.mozilla.org/r/78318/#review76838

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 134 • 8 years ago
mozreview-review

Comment on attachment 8790552 [details]
Bug 1299072: P18. Use MediaResult with InitPromise.

https://reviewboard.mozilla.org/r/78320/#review76842

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 135 • 8 years ago
mozreview-review

Comment on attachment 8790167 [details]
Bug 1299072: P2. Add media decoding error types.

https://reviewboard.mozilla.org/r/78068/#review76844

::: xpcom/base/ErrorList.h:985
(Diff revision 5)
> +  ERROR(NS_ERROR_DOM_MEDIA_END_OF_STREAM,       FAILURE(8)),
> +  ERROR(NS_ERROR_DOM_MEDIA_WAITING_FOR_DATA,    FAILURE(9)),
> +  ERROR(NS_ERROR_DOM_MEDIA_CANCELED,            FAILURE(10)),

A late thought:
Do these non-errors have to be FAILURE's?
If you want to catch them with NS_FAILED(), I guess they have to be.
But if they're only really used internally and handled separately, you may want to make them SUCCESS values.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 136 • 8 years ago
mozreview-review

Comment on attachment 8790553 [details]
Bug 1299072: P19. Pass errors when we failed to create a decoder.

https://reviewboard.mozilla.org/r/78322/#review76846

r+ with micro-nit:

::: dom/media/MediaFormatReader.cpp:404
(Diff revision 1)
>        // EME not supported.
> -      return false;
> +      return MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR, "EME not supported");

The comment is now redundant.

	Jean-Yves Avenard [:jya] Assignee
	Comment 137 • 8 years ago
mozreview-review-reply

Comment on attachment 8790167 [details]
Bug 1299072: P2. Add media decoding error types.

https://reviewboard.mozilla.org/r/78068/#review76844

> A late thought:
> Do these non-errors have to be FAILURE's?
> If you want to catch them with NS_FAILED(), I guess they have to be.
> But if they're only really used internally and handled separately, you may want to make them SUCCESS values.

I believe they do.

They all indicates a failure for one reason or another. But they are "different" errors, they aren't decoding errors as such and those will never be passed to the media element, they only indicate a failure the will require a change of state.

The question is more about if _ERR (which IMHO is redundant based on the name already) 

so for now, I'll leave this as-is.

	Pulsebot
	Comment 139 • 8 years ago

Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d17a9e0123a9
P1. Add message attribute to MediaError object, r=bz,Ehsan,gerald
https://hg.mozilla.org/integration/autoland/rev/a0c433d64e16
P2. Add media decoding error types. r=gerald
https://hg.mozilla.org/integration/autoland/rev/cf57ee925c03
P3. Add MediaResult object. r=gerald
https://hg.mozilla.org/integration/autoland/rev/e62ced7ae6f2
P4. Return extended failure details to reader. r=gerald
https://hg.mozilla.org/integration/autoland/rev/182713015ae3
P5. Don't rely on specific error to assess recoverability. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/933eb4ed5271
P6. Pass decoding error details to MDSM and relatives. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/59b96d2ec6db
P7. Use MediaResult with MetadataPromise. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/16f9471eb78d
P8. Pass decoding error details through SeekTask. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/a17052bc7da4
P9. Pass decoding error details to MediaDecoder. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/f51ca8a5e6c1
P10. Pass decoding error details to media element's error attribute. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/99b6e97356ec
P11. Use MediaResult with AppendBuffer promises. r=gerald
https://hg.mozilla.org/integration/autoland/rev/147b28b58a93
P12. Use MediaResult for MediaDataDemuxer promises. r=gerald
https://hg.mozilla.org/integration/autoland/rev/d22b484668c3
P13. Use MediaResult with TrackBuffersManager internal promises. r=gerald
https://hg.mozilla.org/integration/autoland/rev/ace37342470c
P14. Use MediaResult between TrackBuffersManager and MediaSourceDemuxer. r=gerald
https://hg.mozilla.org/integration/autoland/rev/13ac4ed8d61e
P15. Provide additional error details for most remaining decoders. r=gerald
https://hg.mozilla.org/integration/autoland/rev/991325b4fc60
P16. Add mochitest. r=gerald
https://hg.mozilla.org/integration/autoland/rev/e801d1d6d3ce
P17. Return last decoding error once threshold reached. r=gerald
https://hg.mozilla.org/integration/autoland/rev/9d6ab86f8229
P18. Use MediaResult with InitPromise. r=gerald
https://hg.mozilla.org/integration/autoland/rev/893bd10b1d2f
P19. Pass errors when we failed to create a decoder. r=gerald

	Carsten Book [:Tomcat]
	Comment 140 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d17a9e0123a9
https://hg.mozilla.org/mozilla-central/rev/a0c433d64e16
https://hg.mozilla.org/mozilla-central/rev/cf57ee925c03
https://hg.mozilla.org/mozilla-central/rev/e62ced7ae6f2
https://hg.mozilla.org/mozilla-central/rev/182713015ae3
https://hg.mozilla.org/mozilla-central/rev/933eb4ed5271
https://hg.mozilla.org/mozilla-central/rev/59b96d2ec6db
https://hg.mozilla.org/mozilla-central/rev/16f9471eb78d
https://hg.mozilla.org/mozilla-central/rev/a17052bc7da4
https://hg.mozilla.org/mozilla-central/rev/f51ca8a5e6c1
https://hg.mozilla.org/mozilla-central/rev/99b6e97356ec
https://hg.mozilla.org/mozilla-central/rev/147b28b58a93
https://hg.mozilla.org/mozilla-central/rev/d22b484668c3
https://hg.mozilla.org/mozilla-central/rev/ace37342470c
https://hg.mozilla.org/mozilla-central/rev/13ac4ed8d61e
https://hg.mozilla.org/mozilla-central/rev/991325b4fc60
https://hg.mozilla.org/mozilla-central/rev/e801d1d6d3ce
https://hg.mozilla.org/mozilla-central/rev/9d6ab86f8229
https://hg.mozilla.org/mozilla-central/rev/893bd10b1d2f

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 141 • 8 years ago
mozreview-review

Comment on attachment 8790166 [details]
Bug 1299072: P1. Add message attribute to MediaError object,

https://reviewboard.mozilla.org/r/78066/#review77970

::: dom/webidl/MediaError.webidl:23
(Diff revision 5)
>    const unsigned short MEDIA_ERR_SRC_NOT_SUPPORTED = 4;
>  
>    [Constant]
>    readonly attribute unsigned short code;
> +  [Pref="dom.MediaError.message.enabled"]
> +  readonly attribute DOMString message;

IIRC, there's a commit hook to prevent people from landing patches that change webidl files which don't have a r+ from a DOM peer.

So you need to get a DOM peer to r+ this to land it.

We should also start the process of standardizing this.

	Jean-Yves Avenard [:jya] Assignee
	Comment 142 • 8 years ago
mozreview-review-reply

Comment on attachment 8790166 [details]
Bug 1299072: P1. Add message attribute to MediaError object,

https://reviewboard.mozilla.org/r/78066/#review77970

> IIRC, there's a commit hook to prevent people from landing patches that change webidl files which don't have a r+ from a DOM peer.
> 
> So you need to get a DOM peer to r+ this to land it.
> 
> We should also start the process of standardizing this.

and I did..

From two DOM peers even (bz and ehsan)

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 143 • 8 years ago
mozreview-review

Comment on attachment 8790169 [details]
Bug 1299072: P4. Return extended failure details to reader.

https://reviewboard.mozilla.org/r/78072/#review77974

::: dom/media/MediaFormatReader.h:334
(Diff revision 6)
>      uint32_t mMaxConsecutiveError;
>  
> -    Maybe<MediaDataDecoderError> mError;
> +    Maybe<MediaResult> mError;
>      bool HasFatalError() const
>      {
> -      return mError.isSome() && mError.ref() == MediaDataDecoderError::FATAL_ERROR;
> +      return mError.isSome() && mError.ref() != NS_ERROR_DOM_MEDIA_DECODE_ERR;

It's confusing how you have an explicit fatal error code, and yet here you treat everything except the decode error as fatal.

Maybe you should rename this to HasRecoverableError() or HasNonDecodeError()? Or something else to make it clear you're not actually checking for whether you have NS_ERROR_DOM_MEDIA_FATAL_ERR?

::: dom/media/MediaFormatReader.cpp:1256
(Diff revision 6)
>      DrainDecoder(aTrack);
>      return;
>    }
>  
>    if (decoder.mError &&
> -      decoder.mError.ref() == MediaDataDecoderError::DECODE_ERROR) {
> +      decoder.mError.ref().Code() == NS_ERROR_DOM_MEDIA_DECODE_ERR) {

How come in MediaFormatReader.h you can compare mError.ref() with an nsresult, but here in MediaFormatReader.cpp you need to compare mError.ref().Code() with the nsresult?

i.e. why aren't you consistent in how you compare against nsresult here and in the header?

::: dom/media/platforms/agnostic/OpusDecoder.cpp:158
(Diff revision 6)
>  
>    DecodeError err = DoDecode(aSample);
>    switch (err) {
>      case DecodeError::FATAL_ERROR:
> -      mCallback->Error(MediaDataDecoderError::FATAL_ERROR);
> +      mCallback->Error(MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,
> +                                        __func__));

I thought our convention was to line up on line breaks with the opening bracket/parenthesis. Though I can't see it explicitly mentioned in the Style Guide.

i.e. instead of:

  MediaResult(NS_ERROR_DOM_MEDIA_DECODE_ERR,
                    __func__))
                    
use:

  MediaResult(NS_ERROR_DOM_MEDIA_DECODE_ERR,
              __func__))

And ditto elsewhere.

::: dom/media/platforms/agnostic/gmp/GMPAudioDecoder.cpp:35
(Diff revision 6)
>  {
>    MOZ_ASSERT(IsOnGMPThread());
>  
>    if (aRate == 0 || aChannels == 0) {
>      NS_WARNING("Invalid rate or num channels returned on GMP audio samples");
> -    mCallback->Error(MediaDataDecoderError::FATAL_ERROR);
> +    mCallback->Error(MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__));

There's more than 1 call to Error() in this function, so just passing __func__ to the MediaResult constructor isn't very enlightening. Can you pass the text passed to NS_WARNING() or something else appropriate to disambiguate these different failure paths?

::: dom/media/platforms/agnostic/gmp/GMPVideoDecoder.cpp:187
(Diff revision 6)
>  GMPVideoDecoder::CreateFrame(MediaRawData* aSample)
>  {
>    GMPVideoFrame* ftmp = nullptr;
>    GMPErr err = mHost->CreateFrame(kGMPEncodedVideoFrame, &ftmp);
>    if (GMP_FAILED(err)) {
> -    mCallback->Error(MediaDataDecoderError::FATAL_ERROR);
> +    mCallback->Error(MediaResult(NS_ERROR_OUT_OF_MEMORY, __func__));

We need to disambiguate these two failure points here, so the strings need to be different.

::: dom/media/platforms/apple/AppleATDecoder.cpp:210
(Diff revision 6)
>    if (rv == NS_OK) {
>      for (size_t i = 0; i < mQueuedSamples.Length(); i++) {
> -      if (NS_FAILED(DecodeSample(mQueuedSamples[i]))) {
> +      rv = DecodeSample(mQueuedSamples[i]);
> +      if (NS_FAILED(rv)) {
>          mQueuedSamples.Clear();
> -        mCallback->Error(MediaDataDecoderError::DECODE_ERROR);
> +        mCallback->Error(MediaResult(rv, __func__));

Potentially DecodeSample() could return NS_ERROR_DOM_MEDIA_FATAL_ERR, and that would result in this being ambiguous... Maybe DecodeSample should instead return a MediaResult?

::: dom/media/platforms/apple/AppleVTDecoder.cpp:456
(Diff revision 6)
>    }
>    CMSampleTimingInfo timestamp = TimingInfoFromSample(aSample);
>    rv = CMSampleBufferCreate(kCFAllocatorDefault, block, true, 0, 0, mFormat, 1, 1, &timestamp, 0, NULL, sample.receive());
>    if (rv != noErr) {
>      NS_ERROR("Couldn't create CMSampleBuffer");
> -    return NS_ERROR_FAILURE;
> +    mCallback->Error(MediaResult(NS_ERROR_OUT_OF_MEMORY, __func__));

Again, there are more than one return that have the same nsresult and string, so it would not be possible to disambiguate them.

The purpose of adding extra logging here is for sites to collect these results and return them to us for analysis. So we need to be able to uniquely identify the failure location.

::: dom/media/platforms/gonk/GonkMediaDataDecoder.cpp:193
(Diff revision 6)
>    mLastTime = INT64_MIN;
>    MonitorAutoLock lock(mFlushMonitor);
>    mWaitOutput.Clear();
>    if (mDecoder->flush() != OK) {
>      GMDD_LOG("flush error");
> -    mDecodeCallback->Error(MediaDataDecoderError::FATAL_ERROR);
> +    mDecodeCallback->Error(MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,

Duplicate MediaResults here, but given that this is the Gonk backend, it's not really worth addressing.

::: dom/media/platforms/wrappers/H264Converter.cpp:91
(Diff revision 6)
>      }
>    } else {
>      rv = CheckForSPSChange(aSample);
>    }
>    if (NS_FAILED(rv)) {
> -    mCallback->Error(MediaDataDecoderError::DECODE_ERROR);
> +    mCallback->Error(MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__));

Again, ambiguous errors here; all failures return the same nsresult and string, so remotely it is impossible to determine the failure location.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 144 • 8 years ago
mozreview-review

Comment on attachment 8790175 [details]
Bug 1299072: P10. Pass decoding error details to media element's error attribute.

https://reviewboard.mozilla.org/r/78084/#review77992

::: dom/html/HTMLMediaElement.cpp:4344
(Diff revision 6)
>      ProcessMediaFragmentURI();
>      mDecoder->SetFragmentEndTime(mFragmentEnd);
>    }
>    if (mIsEncrypted) {
>      if (!mMediaSource && Preferences::GetBool("media.eme.mse-only", true)) {
> -      DecodeError();
> +      DecodeError(NS_ERROR_DOM_MEDIA_FATAL_ERR);

This should have an message, as it's possible we'll hit this in the wild.

::: dom/media/eme/MediaKeys.cpp:91
(Diff revision 6)
>    keySessions.Clear();
>    MOZ_ASSERT(mKeySessions.Count() == 0);
>  
>    // Notify the element about that CDM has terminated.
>    if (mElement) {
> -    mElement->DecodeError();
> +    mElement->DecodeError(NS_ERROR_DOM_MEDIA_CDM_ERR);

This definitely needs a message saying that the CDM has crashes, as I believe we do hit this in the wild.

::: dom/media/mediasource/MediaSource.cpp:333
(Diff revision 6)
>    switch (aError.Value()) {
>    case MediaSourceEndOfStreamError::Network:
>      mDecoder->NetworkError();
>      break;
>    case MediaSourceEndOfStreamError::Decode:
> -    mDecoder->DecodeError();
> +    mDecoder->DecodeError(NS_ERROR_DOM_MEDIA_FATAL_ERR);

I expect a message here may be useful.

	Jean-Yves Avenard [:jya] Assignee
	Comment 145 • 8 years ago
mozreview-review-reply

Comment on attachment 8790169 [details]
Bug 1299072: P4. Return extended failure details to reader.

https://reviewboard.mozilla.org/r/78072/#review77974

> It's confusing how you have an explicit fatal error code, and yet here you treat everything except the decode error as fatal.
> 
> Maybe you should rename this to HasRecoverableError() or HasNonDecodeError()? Or something else to make it clear you're not actually checking for whether you have NS_ERROR_DOM_MEDIA_FATAL_ERR?

fatal error is the bucket error for fatal errors that we have no specific details about.
an OOM, overflow etc are fatals.

a decoding error isn't

> How come in MediaFormatReader.h you can compare mError.ref() with an nsresult, but here in MediaFormatReader.cpp you need to compare mError.ref().Code() with the nsresult?
> 
> i.e. why aren't you consistent in how you compare against nsresult here and in the header?

you're just looking at the old code. this was fixed and in a follow up patch completely removed

> I thought our convention was to line up on line breaks with the opening bracket/parenthesis. Though I can't see it explicitly mentioned in the Style Guide.
> 
> i.e. instead of:
> 
>   MediaResult(NS_ERROR_DOM_MEDIA_DECODE_ERR,
>                     __func__))
>                     
> use:
> 
>   MediaResult(NS_ERROR_DOM_MEDIA_DECODE_ERR,
>               __func__))
> 
> And ditto elsewhere.

that was fixed following gerald's comment

> There's more than 1 call to Error() in this function, so just passing __func__ to the MediaResult constructor isn't very enlightening. Can you pass the text passed to NS_WARNING() or something else appropriate to disambiguate these different failure paths?

yes... I have follow up patch that additionally the name of the function that returns the OOM

> We need to disambiguate these two failure points here, so the strings need to be different.

the only duplicate error is for an invalid channel layout, I don't think this can ever happen.

But yes, I have a follow up bug to actually provide further details on all those errors.

	Eric Shepherd [:sheppy]
	Comment 146 • 7 years ago

Documentation has been updated as part of work on documentation updates for bug 1322606.