Closed Bug 1303673 Opened 7 years ago Closed 7 years ago

Provide more detailed error for decoders.

Categories

(Core :: Audio/Video: Playback, defect)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox50 --- unaffected
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(9 files)

Bug 1303673: P1. Add RESULT_DETAIL convenience macro.
58 bytes, text/x-review-board-request
cpearce
: review+
gchang
: approval-mozilla-aurora+
Details
Bug 1303673: P2. Change error code to OOM.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1303673: P3. Provide decryption status in error.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1303673: P4. Provide GMP error code in MediaResult.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1303673: P5. Provide warning when a MediaDataDecoder error occurs.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1303673: P6. Provide further error details for the apple decoders.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1303673: P7. Provide MFT error code.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1303673: P8. Details for the H264 converter.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1303673: P9. Make some GMP errors non fatal.
58 bytes, text/x-review-board-request
cpearce
: review+
Details 
Currently at best the MediaResult contains the function where the error occurred.

However, in many cases, the same function can fail with the same error code multiple times.

Ideally, we should be able to provide exactly where the error occurred. Like giving the line number.
We'll want to uplift that to finish the work done in bug 1299072
Comment on attachment 8792773 [details]
Bug 1303673: P2. Change error code to OOM.

https://reviewboard.mozilla.org/r/79678/#review78388
Attachment #8792773 - Flags: review?(cpearce) → review+
Comment on attachment 8792772 [details]
Bug 1303673: P1. Add RESULT_DETAIL convenience macro.

https://reviewboard.mozilla.org/r/79676/#review78390
Attachment #8792772 - Flags: review?(cpearce) → review+
Comment on attachment 8792774 [details]
Bug 1303673: P3. Provide decryption status in error.

https://reviewboard.mozilla.org/r/79680/#review78392

::: dom/media/platforms/agnostic/eme/EMEDecoderModule.cpp:98
(Diff revision 1)
>        Input(aDecrypted.mSample);
>      } else if (aDecrypted.mStatus != Ok) {
>        if (mCallback) {
> -        mCallback->Error(MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,
> -                                          __func__));
> +        mCallback->Error(MediaResult(
> +          NS_ERROR_DOM_MEDIA_FATAL_ERR,
> +          RESULT_DETAIL("decrypted.mStatus=%u", (unsigned)aDecrypted.mStatus)));

We've previously tried to prefer C++ style casts, i.e. static_cast<unsigned>(...) or unsigned(...) over C style casts. You probably also want to explicitly cast to a 32bit type here.
Attachment #8792774 - Flags: review?(cpearce) → review+
Comment on attachment 8792775 [details]
Bug 1303673: P4. Provide GMP error code in MediaResult.

https://reviewboard.mozilla.org/r/79682/#review78394

::: dom/media/platforms/agnostic/gmp/GMPAudioDecoder.cpp:134
(Diff revision 1)
>  void
>  AudioCallbackAdapter::Terminated()
>  {
> -  NS_WARNING("AAC GMP decoder terminated.");
>    mCallback->Error(MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,
> -                                    __func__));
> +                               RESULT_DETAIL("AAC GMP decoder terminated.")));

s/AAC/Audio/

So it complements the video case below...

::: dom/media/platforms/agnostic/gmp/GMPVideoDecoder.cpp:107
(Diff revision 1)
>  void
>  VideoCallbackAdapter::Terminated()
>  {
>    // Note that this *may* be called from the proxy thread also.
> -  NS_WARNING("GMP decoder terminated.");
> -  mCallback->Error(MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__));
> +  mCallback->Error(MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,
> +                               RESULT_DETAIL("GMP decoder terminated.")));

"Video GMP decoder terminated" so we can distinguish which failed.
Attachment #8792775 - Flags: review?(cpearce) → review+
Comment on attachment 8792776 [details]
Bug 1303673: P5. Provide warning when a MediaDataDecoder error occurs.

https://reviewboard.mozilla.org/r/79684/#review78396
Attachment #8792776 - Flags: review?(cpearce) → review+
Comment on attachment 8792777 [details]
Bug 1303673: P6. Provide further error details for the apple decoders.

https://reviewboard.mozilla.org/r/79686/#review78398

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

Should this be a decode error, rather than the default of fatal (IIRC), so that the MFR can recreate the deoder and retry?

::: dom/media/platforms/apple/AppleVTDecoder.cpp:314
(Diff revision 1)
>      // Lock the returned image data.
>      CVReturn rv = CVPixelBufferLockBaseAddress(aImage, kCVPixelBufferLock_ReadOnly);
>      if (rv != kCVReturnSuccess) {
>        NS_ERROR("error locking pixel data");
> -      mCallback->Error(MediaResult(NS_ERROR_OUT_OF_MEMORY, __func__));
> -      return NS_ERROR_OUT_OF_MEMORY;
> +      mCallback->Error(
> +        MediaResult(NS_ERROR_DOM_MEDIA_DECODE_ERR,

Identifying this as a decode error means we'll recreate the decoder right? And after a bunch of failures in a row, we give up right?

How confident are you that we can recover from this by re-trying?
Attachment #8792777 - Flags: review?(cpearce) → review+
Comment on attachment 8792778 [details]
Bug 1303673: P7. Provide MFT error code.

https://reviewboard.mozilla.org/r/79688/#review78400
Attachment #8792778 - Flags: review?(cpearce) → review+
Comment on attachment 8792779 [details]
Bug 1303673: P8. Details for the H264 converter.

https://reviewboard.mozilla.org/r/79690/#review78404
Attachment #8792779 - Flags: review?(cpearce) → review+
Comment on attachment 8792780 [details]
Bug 1303673: P9. Make some GMP errors non fatal.

https://reviewboard.mozilla.org/r/79692/#review78406

Very nice. Thanks for doing all this!
Attachment #8792780 - Flags: review?(cpearce) → review+
Comment on attachment 8792777 [details]
Bug 1303673: P6. Provide further error details for the apple decoders.

https://reviewboard.mozilla.org/r/79686/#review78398

> Should this be a decode error, rather than the default of fatal (IIRC), so that the MFR can recreate the deoder and retry?

this is the nsresult value returned by AppleATDecoder::DecodeSample, which is already set to the proper value (OOM, overflow, decode error etc.)

> Identifying this as a decode error means we'll recreate the decoder right? And after a bunch of failures in a row, we give up right?
> 
> How confident are you that we can recover from this by re-trying?

well, seeing that it's a locking error on a buffer. I have no idea if we can recover. I don't believe this can ever happen. It wasn't an OOM that's for sure.
Comment on attachment 8792774 [details]
Bug 1303673: P3. Provide decryption status in error.

https://reviewboard.mozilla.org/r/79680/#review78392

> We've previously tried to prefer C++ style casts, i.e. static_cast<unsigned>(...) or unsigned(...) over C style casts. You probably also want to explicitly cast to a 32bit type here.

what about uint32_t(blah) is that c++ enough ?
Comment on attachment 8792775 [details]
Bug 1303673: P4. Provide GMP error code in MediaResult.

https://reviewboard.mozilla.org/r/79682/#review78394

> "Video GMP decoder terminated" so we can distinguish which failed.

the macro automatatically adds the function name in there.. but sure. will do.
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7634e43655f8
P1. Add RESULT_DETAIL convenience macro. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/cd7bd96b4655
P2. Change error code to OOM. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/5499543bde09
P3. Provide decryption status in error. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/317254ae5d28
P4. Provide GMP error code in MediaResult. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/3f54154cd702
P5. Provide warning when a MediaDataDecoder error occurs. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/3b5d65c68be3
P6. Provide further error details for the apple decoders. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/3bb709c3ab10
P7. Provide MFT error code. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/fd7aa6fdf423
P8. Details for the H264 converter. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/37123fd61b52
P9. Make some GMP errors non fatal. r=cpearce
Blocks: 1304252
Comment on attachment 8792772 [details]
Bug 1303673: P1. Add RESULT_DETAIL convenience macro.

Approval Request Comment
[Feature/regressing bug #]: None
[User impact if declined]: Making harder to troubleshoot current decoding error seen with amazon and netflix
[Describe test coverage new/current, TreeHerder]: In central
[Risks and why]: None. We're just adding details to help identify where failures occur
[String/UUID change made/needed]: None
Attachment #8792772 - Flags: approval-mozilla-aurora?
Forgot to add that the request is for all patches.

Bug 1304252 will follow
Hi :jya,
Is the uplift request for all P1~P9 or P1 only?
Flags: needinfo?(jyavenard)
All of them as per comment 34. 

Thank you.
Flags: needinfo?(jyavenard)
Comment on attachment 8792772 [details]
Bug 1303673: P1. Add RESULT_DETAIL convenience macro.

This patch provides more detailed error information for troubleshooting. Take it in 51 aurora.
Attachment #8792772 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.