Closed Bug 1397141 Opened 7 years ago Closed 7 years ago

Report more detailed error description when creating decoder fail

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

Other Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

Details

Attachments

(8 files, 4 obsolete files)

59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
mattwoodrow
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
59 bytes, text/x-review-board-request
jya
: review+
Details
Fork from bug1396547 comment10, we should report more detailed error description to decoder doctor, instead of just showing "can not create decoder".
See Also: → 1379928
Attachment #8904960 - Attachment is obsolete: true
Attachment #8904961 - Attachment is obsolete: true
Attachment #8904962 - Attachment is obsolete: true
Attachment #8904963 - Attachment is obsolete: true
Comment on attachment 8905395 [details]
Bug 1397141 - part1 : update error description for getting more details

https://reviewboard.mozilla.org/r/177182/#review182540

::: commit-message-c9593:3
(Diff revision 1)
> +Bug 1397141 - part1 : update error description when resolution is not supported.
> +
> +We should report the more detailed error when create decoder fail, instead of

when creating the decoder failed

::: dom/media/platforms/wmf/WMFDecoderModule.cpp:113
(Diff revision 1)
>      new WMFVideoMFTManager(aParams.VideoConfig(),
>                             aParams.mKnowsCompositor,
>                             aParams.mImageContainer,
>                             sDXVAEnabled));
>  
> -  if (!manager->Init()) {
> +  if (!manager->Init(aParams.mError)) {

update aParams.mError from the MediaResult returned by Init instead

::: dom/media/platforms/wmf/WMFVideoMFTManager.h:10
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #if !defined(WMFVideoMFTManager_h_)
>  #define WMFVideoMFTManager_h_
>  
> +#include "MediaResult.h"

ascii alphabetical order please

::: dom/media/platforms/wmf/WMFVideoMFTManager.h:32
(Diff revision 1)
>                       layers::KnowsCompositor* aKnowsCompositor,
>                       layers::ImageContainer* aImageContainer,
>                       bool aDXVAEnabled);
>    ~WMFVideoMFTManager();
>  
> -  bool Init();
> +  bool Init(MediaResult* aError);

make init return a MediaResult instead.

You can then check if it failed with if (NS_FAILED(result))

::: dom/media/platforms/wmf/WMFVideoMFTManager.h:67
(Diff revision 1)
>             ? MediaDataDecoder::ConversionRequired::kNeedAnnexB
>             : MediaDataDecoder::ConversionRequired::kNeedNone;
>    }
>  
>  private:
> -  bool ValidateVideoInfo();
> +  bool ValidateVideoInfo(MediaResult* aError);

return a MediaResult here, no need for an output parameter

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:577
(Diff revision 1)
>  }
>  
>  bool
> -WMFVideoMFTManager::Init()
> +WMFVideoMFTManager::Init(MediaResult* aError)
>  {
> -  if (!ValidateVideoInfo()) {
> +  if (!ValidateVideoInfo(aError)) {

instead you do:

MediaResult result = ValidateVideoInfo();
if (NS_FAILED(result)) {
  return result;
}
Attachment #8905395 - Flags: review?(jyavenard) → review+
Comment on attachment 8905396 [details]
Bug 1397141 - part2 : move resolution constraints to WMFVideoMFTManager

https://reviewboard.mozilla.org/r/177184/#review182550

::: commit-message-8967a:1
(Diff revision 1)
> +Bug 1397141 - part2 : remove the minimum resolution constraint

this change doesn't do what the title describe.

It's also no longer necessary to do those steps with bug 1392143 as HW decoding is never used now for resolution < 132 pixels.

Additionally, this code wouldn't have worked anyway.

The issue seen with video < 48 pixels high wasn't that we couldn't create the decoder.

but that Input() would never return.
You wouldn't have detected the broken configuration simply by checking if the HW decoder could be created or not
Attachment #8905396 - Flags: review?(jyavenard) → review-
Comment on attachment 8905397 [details]
Bug 1397141 - part3 : remove the minimum resolution check.

https://reviewboard.mozilla.org/r/177186/#review182552

::: dom/media/platforms/wrappers/H264Converter.h:108
(Diff revision 1)
>    bool mNeedKeyframe = true;
>    const TrackInfo::TrackType mType;
>    MediaEventProducer<TrackInfo::TrackType>* const mOnWaitingForKeyEvent;
>    const CreateDecoderParams::OptionSet mDecoderOptions;
>    Maybe<bool> mCanRecycleDecoder;
> +  MediaResult* mError;

I would have reused mLastError instead. changing the type from nsresult to MediaResult.

and this bug does nothing, compiled as is it would fail with warning-as-error as the member mError is unused
Attachment #8905397 - Flags: review?(jyavenard) → review-
Comment on attachment 8905397 [details]
Bug 1397141 - part3 : remove the minimum resolution check.

https://reviewboard.mozilla.org/r/177186/#review182552

> I would have reused mLastError instead. changing the type from nsresult to MediaResult.
> 
> and this bug does nothing, compiled as is it would fail with warning-as-error as the member mError is unused

my bad, missed a line. shouldn't review change after midnight sorry.
Comment on attachment 8905397 [details]
Bug 1397141 - part3 : remove the minimum resolution check.

https://reviewboard.mozilla.org/r/177186/#review182558
Attachment #8905397 - Flags: review- → review+
Comment on attachment 8905397 [details]
Bug 1397141 - part3 : remove the minimum resolution check.

https://reviewboard.mozilla.org/r/177186/#review182560

::: dom/media/platforms/wrappers/H264Converter.cpp:285
(Diff revision 1)
>      mKnowsCompositor,
>      mGMPCrashHelper,
>      mType,
>      mOnWaitingForKeyEvent,
> -    mDecoderOptions
> +    mDecoderOptions,
> +    mError

actually no, this is bad. we have mError potentially accessed on two different threads.t more importantly *after* the MediaResult has been destroyed

H264::CreateDecoder can be called on the reader's taskqueue in the constructor.

And if for annex B, lazily.
By that time aParams.mError would have been destroyed, and you would now do a UAF
Attachment #8905397 - Flags: review+ → review-
Comment on attachment 8905398 [details]
Bug 1397141 - part4 : change mLastError type to MediaResult

https://reviewboard.mozilla.org/r/177188/#review182564

DecoderParam uses raw pointers to its argument. Once the function return, most arguments can no longer be used, you certainly can't access the pointer to the MediaResult. MediaResult isn't refcounted either.

So by the time you've finished calling CreateDecoder() aParams.mError is no longer guaranteed to exist, and in this case it certainly isn't.

The RemoteVideoDecoder is asynchronous, you don't know when the ipdl will return with the initialisation result.

This is UAF

::: dom/media/MediaResult.h:25
(Diff revision 1)
>  namespace mozilla {
>  
>  class MediaResult
>  {
>  public:
> +  MediaResult()

I explicitly designed MediaResult so that it didn't have a default constructor on purpose.

it needs to be initialised. you can use MediaResult(NS_OK)
Attachment #8905398 - Flags: review?(jyavenard) → review-
Comment on attachment 8905398 [details]
Bug 1397141 - part4 : change mLastError type to MediaResult

https://reviewboard.mozilla.org/r/177188/#review182566

::: dom/media/ipc/VideoDecoderParent.cpp:87
(Diff revision 1)
>    mDecoder = pdm->CreateVideoDecoder(params);
>  #else
>    MOZ_ASSERT(false,
>               "Can't use RemoteVideoDecoder on non-Windows platforms yet");
>  #endif
> -
> +  *aErrorDescription = error.Description();

you should only update the value if error is NS_FAILED() I think
Comment on attachment 8905396 [details]
Bug 1397141 - part2 : move resolution constraints to WMFVideoMFTManager

https://reviewboard.mozilla.org/r/177184/#review182550

> this change doesn't do what the title describe.
> 
> It's also no longer necessary to do those steps with bug 1392143 as HW decoding is never used now for resolution < 132 pixels.
> 
> Additionally, this code wouldn't have worked anyway.
> 
> The issue seen with video < 48 pixels high wasn't that we couldn't create the decoder.
> 
> but that Input() would never return.
> You wouldn't have detected the broken configuration simply by checking if the HW decoder could be created or not

I'll split this patch as two parts, one is moving the constriant checking to WMFVideoManager, another part is removing the minimum resolution checking.

We still need to have maximum resolution constraints.
Comment on attachment 8905397 [details]
Bug 1397141 - part3 : remove the minimum resolution check.

https://reviewboard.mozilla.org/r/177186/#review182560

> actually no, this is bad. we have mError potentially accessed on two different threads.t more importantly *after* the MediaResult has been destroyed
> 
> H264::CreateDecoder can be called on the reader's taskqueue in the constructor.
> 
> And if for annex B, lazily.
> By that time aParams.mError would have been destroyed, and you would now do a UAF

I think using LastError is a good idea, it can avoid UAF.

If stream is AVCC, we create decoder in H264Converter's ctor. 
Unfortunately, if creating decoder failed, PDMFactory can know it via H264Converter::GetLastError(), and then return error back to MFR.

If the stream is AnnexB or we can't create decoder in first time because lacking of the sps, we would create and init decoder lazily in H264Converter::Decode(). 
If creating decoder failed, we could return the detailed error description with rejected decode promise.

---

"mError potentially accessed on two different threads", do you mean reader's task queue and VideoDecoderManagerChild's thread?
(In reply to Jean-Yves Avenard [:jya] from comment #15)
> Comment on attachment 8905398 [details]
> Bug 1397141 - part4 : update error description from GPU process.
> 
> https://reviewboard.mozilla.org/r/177188/#review182564
> 
> DecoderParam uses raw pointers to its argument. Once the function return,
> most arguments can no longer be used, you certainly can't access the pointer
> to the MediaResult. MediaResult isn't refcounted either.
> 
> So by the time you've finished calling CreateDecoder() aParams.mError is no
> longer guaranteed to exist, and in this case it certainly isn't.
> 
> The RemoteVideoDecoder is asynchronous, you don't know when the ipdl will
> return with the initialisation result.
> 
> This is UAF

Not sure I understand correctly what you said, but the ipdl is a synchronous task [1]. 

So, if the |aParams.mError| exists in RemoteDecoderModule::CreateVideoDecoder(), it would also exist on the following call stack, is that right?

[1] http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/dom/media/ipc/RemoteVideoDecoder.cpp#182
Flags: needinfo?(jyavenard)
Comment on attachment 8905397 [details]
Bug 1397141 - part3 : remove the minimum resolution check.

https://reviewboard.mozilla.org/r/177186/#review182560

> I think using LastError is a good idea, it can avoid UAF.
> 
> If stream is AVCC, we create decoder in H264Converter's ctor. 
> Unfortunately, if creating decoder failed, PDMFactory can know it via H264Converter::GetLastError(), and then return error back to MFR.
> 
> If the stream is AnnexB or we can't create decoder in first time because lacking of the sps, we would create and init decoder lazily in H264Converter::Decode(). 
> If creating decoder failed, we could return the detailed error description with rejected decode promise.
> 
> ---
> 
> "mError potentially accessed on two different threads", do you mean reader's task queue and VideoDecoderManagerChild's thread?

With the H264Converter error can be requested with GetLastError has you mentioned. So just re-use that.

Right now it's a nsresult object, but if you made that a MediaResult you'll get all the information you need there. You'll also need to modify H264Converter::CreateDecoder to return a MediaResult object and replace all instance of nsresult in H264Converter with MediaResult.

and pass on whatever value is returned to either the DecodePromise or the InitPromise.

You can't use the mError provided in DecoderParam, the pointer to mError could be dangling.
Got some test case fails, will ask for a review again after fixing the problem.
Flags: needinfo?(jyavenard)
Comment on attachment 8905396 [details]
Bug 1397141 - part2 : move resolution constraints to WMFVideoMFTManager

https://reviewboard.mozilla.org/r/177184/#review183212

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:572
(Diff revision 2)
> +  const bool Is4KCapable = IsWin8OrLater() || IsWin7H264Decoder4KCapable();
>    static const int32_t MIN_H264_FRAME_DIMENSION = 48;
> +  static const int32_t MAX_H264_FRAME_WIDTH = Is4KCapable ? 4096 : 1920;
> +  static const int32_t MAX_H264_FRAME_HEIGHT = Is4KCapable ? 2304 : 1088;
> +
> +  if (mStreamType != H264 ||

please move this before static the constants above.

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:574
(Diff revision 2)
> +  static const int32_t MAX_H264_FRAME_WIDTH = Is4KCapable ? 4096 : 1920;
> +  static const int32_t MAX_H264_FRAME_HEIGHT = Is4KCapable ? 2304 : 1088;
> +
> +  if (mStreamType != H264 ||
> +      gfxPrefs::PDMWMFAllowUnsupportedResolutions()) {
> +    return MediaResult(NS_OK);

return NS_OK;

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:577
(Diff revision 2)
> +  if (mStreamType != H264 ||
> +      gfxPrefs::PDMWMFAllowUnsupportedResolutions()) {
> +    return MediaResult(NS_OK);
> +  }
> +
> +  // TODO : remove minimum resolution check in following patch.

please remove this TODO

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:595
(Diff revision 2)
> +    return MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,
> +                       RESULT_DETAIL("Can't decode H.264 stream because its "
> +                                     "resolution is out of the maximum limitation"));
> +  }
> +
>    return MediaResult(NS_OK);

return NS_OK;

will do
Attachment #8905396 - Flags: review?(jyavenard) → review+
Comment on attachment 8905396 [details]
Bug 1397141 - part2 : move resolution constraints to WMFVideoMFTManager

https://reviewboard.mozilla.org/r/177184/#review183214

::: commit-message-8967a:1
(Diff revision 2)
> +Bug 1397141 - part2 : move the resolution constraint to WMFVideoMFTManager

move resolution constraints to WMFVideoMFTManager

::: commit-message-8967a:4
(Diff revision 2)
> +Bug 1397141 - part2 : move the resolution constraint to WMFVideoMFTManager
> +
> +WMFDecoderModule should only focus on whether the mime type is supported or not.
> +Let WMFVideoMFTManager do the furtherchecking.

Let WMFVideoMFTManager do the checking
Comment on attachment 8905397 [details]
Bug 1397141 - part3 : remove the minimum resolution check.

https://reviewboard.mozilla.org/r/177186/#review183216

::: commit-message-4caaa:1
(Diff revision 2)
> +Bug 1397141 - part3 : remove the minimum resolution checking.

remove the minimum resolution check

::: commit-message-4caaa:3
(Diff revision 2)
> +Bug 1397141 - part3 : remove the minimum resolution checking.
> +
> +After bug1392143, we won't enable HW decoding for the resolution < 132 pixels.

space between bug and 1392143

::: commit-message-4caaa:4
(Diff revision 2)
> +Bug 1397141 - part3 : remove the minimum resolution checking.
> +
> +After bug1392143, we won't enable HW decoding for the resolution < 132 pixels.
> +In addition, software decoder doesn't have the minimum resolution limitation, so

s/limitation/limit

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:563
(Diff revision 2)
>  
>  MediaResult
>  WMFVideoMFTManager::ValidateVideoInfo()
>  {
> -  // The WMF H.264 decoder is documented to have a minimum resolution
> -  // 48x48 pixels. We've observed the decoder working for output smaller than
> +  // The WMF H.264 decoder is documented to have a minimum resolution 48x48 pixels
> +  // for hw decoder, but we won't enable hw decoding for the resolution < 132 pixels.

for resolution

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:564
(Diff revision 2)
>  MediaResult
>  WMFVideoMFTManager::ValidateVideoInfo()
>  {
> -  // The WMF H.264 decoder is documented to have a minimum resolution
> -  // 48x48 pixels. We've observed the decoder working for output smaller than
> -  // that, but on some output it hangs in IMFTransform::ProcessOutput(), so
> +  // The WMF H.264 decoder is documented to have a minimum resolution 48x48 pixels
> +  // for hw decoder, but we won't enable hw decoding for the resolution < 132 pixels.
> +  // Software decoder doesn't have the minimum limitation, but it still has

it is assumed the software decoder doesn't have this limitation but ...

(personally, I doubt there's a maximum resolution as well)
Attachment #8905397 - Flags: review?(jyavenard) → review+
Comment on attachment 8905398 [details]
Bug 1397141 - part4 : change mLastError type to MediaResult

https://reviewboard.mozilla.org/r/177188/#review183220

::: commit-message-67369:1
(Diff revision 2)
> +Bug 1397141 - part4 : change mLastError to MediaResult type

change mLastError type to MediaResult

::: dom/media/platforms/wrappers/H264Converter.cpp:263
(Diff revision 2)
>      // Do some format check here.
>      // WMF H.264 Video Decoder and Apple ATDecoder do not support YUV444 format.
>      if (spsdata.profile_idc == 244 /* Hi444PP */ ||
>          spsdata.chroma_format_idc == PDMFactory::kYUV444) {
> -      mLastError = NS_ERROR_FAILURE;
> +      mLastError = MediaResult(NS_ERROR_FAILURE,
> +                               RESULT_DETAIL("Not support YUV444 format."));

"No support for YUV444 format"

::: dom/media/platforms/wrappers/H264Converter.cpp:271
(Diff revision 2)
>        }
>        return NS_ERROR_FAILURE;
>      }
>    } else {
> -    // SPS was invalid.
> -    mLastError = NS_ERROR_FAILURE;
> +    mLastError = MediaResult(NS_ERROR_FAILURE,
> +                             RESULT_DETAIL("SPS was invalid."));

"Invalid SPS NAL"

::: dom/media/platforms/wrappers/H264Converter.cpp:290
(Diff revision 2)
> +    &mLastError
>    });
>  
>    if (!mDecoder) {
> -    mLastError = NS_ERROR_FAILURE;
> +    MOZ_ASSERT(NS_FAILED(mLastError));
> +    mLastError = MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,

use mLastError.mCode

otherwise you've broken AVC3 support, as now mLastError can never be NS_ERROR_NOT_INITIALIZED
Attachment #8905398 - Flags: review?(jyavenard) → review+
Comment on attachment 8905398 [details]
Bug 1397141 - part4 : change mLastError type to MediaResult

https://reviewboard.mozilla.org/r/177188/#review183220

> use mLastError.mCode
> 
> otherwise you've broken AVC3 support, as now mLastError can never be NS_ERROR_NOT_INITIALIZED

you mustn't have used a try run here, as this would have failed in dom/media/mediasource/test, and if not we have a problem in our test
Attachment #8906549 - Flags: review?(matt.woodrow)
Comment on attachment 8906549 [details]
Bug 1397141 - part5 : update error description from GPU process.

https://reviewboard.mozilla.org/r/177668/#review183228

LGTM

::: dom/media/ipc/VideoDecoderChild.cpp:199
(Diff revision 1)
>    // leave us in an error state. We'll then immediately reject the promise when
>    // Init() is called and the caller can try again. Hopefully by then the new
>    // manager is ready, or we've notified the caller of it being no longer
>    // available. If not, then the cycle repeats until we're ready.
>    if (!manager->CanSend()) {
> -    return true;
> +    return MediaResult(NS_OK);

return NS_OK;
Attachment #8906549 - Flags: review?(jyavenard) → review+
Comment on attachment 8906550 [details]
Bug 1397141 - part6 : use MediaResult to replace nsresult

https://reviewboard.mozilla.org/r/177670/#review183230

I'd like to see another round, where mLastError isn't set on the fly within the function.

now that you're returning MediaResult, there's no longer a need to set it within each function. mLastError if creating the decoder failed in the H264Converter constructor.

so doing mLastError = CreateDecoder(...) is much better. Everywhere else, the error will always be returned through the various promises.

::: dom/media/platforms/wrappers/H264Converter.cpp:84
(Diff revision 1)
>        MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,
>                    RESULT_DETAIL("Invalid H264 content")),
>        __func__);
>    }
>  
> -  nsresult rv;
> +  MediaResult result(NS_OK);

constructing MediaResult is unecessary as it will always be set after.

::: dom/media/platforms/wrappers/H264Converter.cpp:84
(Diff revision 1)
>        MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,
>                    RESULT_DETAIL("Invalid H264 content")),
>        __func__);
>    }
>  
> -  nsresult rv;
> +  MediaResult result(NS_OK);

I'd keep the name rv, rv stands for return value, so it's still just that.

::: dom/media/platforms/wrappers/H264Converter.cpp:116
(Diff revision 1)
>      return p;
>    }
>  
> -  if (NS_FAILED(rv)) {
> +  if (NS_FAILED(result)) {
>      return DecodePromise::CreateAndReject(
>        MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,

same as P2, you got result to the value you want, you why change the MediaResult::mCode ?

::: dom/media/platforms/wrappers/H264Converter.cpp:255
(Diff revision 1)
>  {
>    if (!mp4_demuxer::H264::HasSPS(aConfig.mExtraData)) {
>      // nothing found yet, will try again later
> -    return NS_ERROR_NOT_INITIALIZED;
> +    mLastError = MediaResult(NS_ERROR_NOT_INITIALIZED,
> +                             RESULT_DETAIL("Doesn't have SPS, will try again later."));
> +    return mLastError;

Remove all the settings of mLastError within that code.

instead it should be something like mLastError = CreateDecoder(...) in the constructor.

::: dom/media/platforms/wrappers/H264Converter.cpp:430
(Diff revision 1)
>      // This scenario can only occur on Android with devices that can recycle a
>      // decoder.
>      if (!mp4_demuxer::H264::HasSPS(aSample->mExtraData) ||
>          mp4_demuxer::H264::CompareExtraData(aSample->mExtraData,
>                                              mOriginalExtraData)) {
> -      return NS_OK;
> +      return MediaResult(NS_OK);

return NS_OK;

same wherever you return MediaResult(NS_OK);

::: dom/media/platforms/wrappers/H264Converter.cpp:453
(Diff revision 1)
>    }
>  
>    // The SPS has changed, signal to drain the current decoder and once done
>    // create a new one.
>    DrainThenFlushDecoder(aSample);
> -  return NS_ERROR_DOM_MEDIA_INITIALIZING_DECODER;
> +  mLastError = MediaResult(NS_ERROR_DOM_MEDIA_INITIALIZING_DECODER,

why do you set mLastError here?
this isn't an error per say, it shouldn't be returned to the H264Converter client
Attachment #8906550 - Flags: review?(jyavenard) → review-
Comment on attachment 8906551 [details]
Bug 1397141 - part7 : update error description in MFR.

https://reviewboard.mozilla.org/r/177672/#review183232

::: dom/media/MediaFormatReader.cpp:730
(Diff revision 1)
>        MOZ_ASSERT(mOwner->mCDMProxy);
>        mOwner->mPlatform->SetCDMProxy(mOwner->mCDMProxy);
>      }
>    }
>  
> +  MediaResult result = MediaResult(NS_OK);

result(NS_OK);

or result = NS_OK;
Attachment #8906551 - Flags: review?(jyavenard) → review+
Comment on attachment 8906552 [details]
Bug 1397141 - part8 : update test for video under 48x48.

https://reviewboard.mozilla.org/r/177674/#review183234

please don't remove this test, instead we should check that it never fails.
Attachment #8906552 - Flags: review?(jyavenard) → review-
Priority: -- → P3
Comment on attachment 8906549 [details]
Bug 1397141 - part5 : update error description from GPU process.

https://reviewboard.mozilla.org/r/177668/#review183310
Attachment #8906549 - Flags: review?(matt.woodrow) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #34)
> Comment on attachment 8905398 [details]
> Bug 1397141 - part4 : change mLastError to MediaResult type
> 
> https://reviewboard.mozilla.org/r/177188/#review183220
> 
> > use mLastError.mCode
> > 
> > otherwise you've broken AVC3 support, as now mLastError can never be NS_ERROR_NOT_INITIALIZED
> you mustn't have used a try run here, as this would have failed in
> dom/media/mediasource/test, and if not we have a problem in our test

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7d41360abd58fa3c356cab768cb021760d28553&selectedJob=130023905

No error.
(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #40)
> > > otherwise you've broken AVC3 support, as now mLastError can never be NS_ERROR_NOT_INITIALIZED

It would be set in patch6.
Comment on attachment 8906550 [details]
Bug 1397141 - part6 : use MediaResult to replace nsresult

https://reviewboard.mozilla.org/r/177670/#review183690

::: dom/media/platforms/wrappers/H264Converter.cpp:84
(Diff revisions 1 - 2)
>        MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,
>                    RESULT_DETAIL("Invalid H264 content")),
>        __func__);
>    }
>  
> -  MediaResult result(NS_OK);
> +  MediaResult rv(NS_OK);

keep the default constructor.

especially as rv will be assigned later.

::: dom/media/platforms/wrappers/H264Converter.cpp:118
(Diff revisions 1 - 2)
>  
> -  if (NS_FAILED(result)) {
> +  if (NS_FAILED(rv)) {
>      return DecodePromise::CreateAndReject(
> -      MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,
> +      MediaResult(rv.Code(),
>                    RESULT_DETAIL("Unable to create H264 decoder, reason = %s.",
> -                                result.Description().get())),
> +                                mLastError.Description().get())),

why use mLastError, what's the point of returning rv all the way?
especially now that mLastError isn't even set.

Just reject the promise with rv, no need to create a new MediaResult

::: dom/media/platforms/wrappers/H264Converter.cpp:308
(Diff revision 2)
>    RefPtr<MediaByteBuffer> extra_data =
>      mp4_demuxer::H264::ExtractExtraData(aSample);
>    bool inbandExtradata = mp4_demuxer::H264::HasSPS(extra_data);
>    if (!inbandExtradata &&
>        !mp4_demuxer::H264::HasSPS(mCurrentConfig.mExtraData)) {
> -    return NS_ERROR_NOT_INITIALIZED;
> +    return MediaResult(NS_ERROR_NOT_INITIALIZED,

keep that as-is.

no need to construct a MediaResult with more information, this MediaResult is never passed back to the user.

::: dom/media/platforms/wrappers/H264Converter.cpp:316
(Diff revision 2)
>  
>    if (inbandExtradata) {
>      UpdateConfigFromExtraData(extra_data);
>    }
>  
> -  nsresult rv =
> +  mLastError = CreateDecoder(mCurrentConfig,

MediaResult rv

there's no point setting mLastError anymore, as it will be overwritten in the constructor anyway.

::: dom/media/platforms/wrappers/H264Converter.cpp:357
(Diff revision 2)
>              __func__);
>          })
>        ->Track(mInitPromiseRequest);
> -    return NS_ERROR_DOM_MEDIA_INITIALIZING_DECODER;
> +    return MediaResult(NS_ERROR_DOM_MEDIA_INITIALIZING_DECODER,
> +                       RESULT_DETAIL("Can't initialize decoder."));
>    }

keep that as-is

::: dom/media/platforms/wrappers/H264Converter.cpp:447
(Diff revision 2)
>    }
>  
>    // The SPS has changed, signal to drain the current decoder and once done
>    // create a new one.
>    DrainThenFlushDecoder(aSample);
> -  return NS_ERROR_DOM_MEDIA_INITIALIZING_DECODER;
> +  return MediaResult(NS_ERROR_DOM_MEDIA_INITIALIZING_DECODER,

keep that as-is.
Again, none of this is returned to the user, constructing a full blown MediaResult serves no purpose.

Both NS_ERROR_DOM_MEDIA_INITIALIZING_DECODER and NS_ERROR_NOT_INITIALIZED are special return code only used internally
Attachment #8906550 - Flags: review?(jyavenard) → review-
Comment on attachment 8906552 [details]
Bug 1397141 - part8 : update test for video under 48x48.

https://reviewboard.mozilla.org/r/177674/#review183698

::: commit-message-d09f7:2
(Diff revision 2)
> +Bug 1397141 - part8 : update test for video under 48x48.
> +

please provide further details in your commit message.

what it now does and why.
Attachment #8906552 - Flags: review?(jyavenard) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #51)
> Comment on attachment 8906550 [details]
> Bug 1397141 - part6 : use MediaResult to replace nsresult
> 
> https://reviewboard.mozilla.org/r/177670/#review183690
> 
> ::: dom/media/platforms/wrappers/H264Converter.cpp:84
> (Diff revisions 1 - 2)
> >        MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,
> >                    RESULT_DETAIL("Invalid H264 content")),
> >        __func__);
> >    }
> >  
> > -  MediaResult result(NS_OK);
> > +  MediaResult rv(NS_OK);
> 
> keep the default constructor.
> 
> especially as rv will be assigned later.

The fact is that we don't have this kind of ctor... and you said that we should not add it in comment15.

http://searchfox.org/mozilla-central/source/dom/media/MediaResult.h#22
Flags: needinfo?(jyavenard)
Comment on attachment 8906550 [details]
Bug 1397141 - part6 : use MediaResult to replace nsresult

https://reviewboard.mozilla.org/r/177670/#review183738

::: dom/media/platforms/wrappers/H264Converter.cpp:262
(Diff revision 3)
> -      mLastError = MediaResult(NS_ERROR_FAILURE,
> -                               RESULT_DETAIL("Not support for YUV444 format."));
>        if (aDiagnostics) {
>          aDiagnostics->SetVideoNotSupported();
>        }
> -      return NS_ERROR_FAILURE;
> +      return MediaResult(NS_ERROR_FAILURE,

make this NS_ERROR_DOM_MEDIA_FATAL_ERR otherwise the MFR will retry unecessarily 3 times

::: dom/media/platforms/wrappers/H264Converter.cpp:263
(Diff revision 3)
> -                               RESULT_DETAIL("Not support for YUV444 format."));
>        if (aDiagnostics) {
>          aDiagnostics->SetVideoNotSupported();
>        }
> -      return NS_ERROR_FAILURE;
> +      return MediaResult(NS_ERROR_FAILURE,
> +                         RESULT_DETAIL("Not support for YUV444 format."));

May as well fix the grammar.

"No support for YUV444 format"
Attachment #8906550 - Flags: review?(jyavenard) → review+
Need rebase.
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f22cc93383c
part1 : update error description for getting more details r=jya
https://hg.mozilla.org/integration/autoland/rev/81a987374ac7
part2 : move resolution constraints to WMFVideoMFTManager r=jya
https://hg.mozilla.org/integration/autoland/rev/57ab4c51c72b
part3 : remove the minimum resolution check. r=jya
https://hg.mozilla.org/integration/autoland/rev/9140f22570ea
part4 : change mLastError type to MediaResult r=jya
https://hg.mozilla.org/integration/autoland/rev/6dade48b3326
part5 : update error description from GPU process. r=jya,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/c78c096d0be9
part6 : use MediaResult to replace nsresult r=jya
https://hg.mozilla.org/integration/autoland/rev/eab2e401c60f
part7 : update error description in MFR. r=jya
https://hg.mozilla.org/integration/autoland/rev/f6f5863e50a8
part8 : update test for video under 48x48. r=jya
Sorry, forget to move this code into windows-only part.

New try result for build on other platforms.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb72d5ac02b912b22891e5d0f0f3217b8d32f56b
Flags: needinfo?(alwu)
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81e046d67cae
part1 : update error description for getting more details r=jya
https://hg.mozilla.org/integration/autoland/rev/f67f016f5688
part2 : move resolution constraints to WMFVideoMFTManager r=jya
https://hg.mozilla.org/integration/autoland/rev/81a7bf2ee098
part3 : remove the minimum resolution check. r=jya
https://hg.mozilla.org/integration/autoland/rev/57edd7246a04
part4 : change mLastError type to MediaResult r=jya
https://hg.mozilla.org/integration/autoland/rev/5af7f08bcaab
part5 : update error description from GPU process. r=jya,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/14249cad9e8e
part6 : use MediaResult to replace nsresult r=jya
https://hg.mozilla.org/integration/autoland/rev/578f746a2721
part7 : update error description in MFR. r=jya
https://hg.mozilla.org/integration/autoland/rev/784819df6f88
part8 : update test for video under 48x48. r=jya
You need to log in before you can comment on or make changes to this bug.