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)
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".
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8904960 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8904961 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8904962 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8904963 -
Attachment is obsolete: true
Comment 9•7 years ago
|
||
mozreview-review |
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 10•7 years ago
|
||
mozreview-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 11•7 years ago
|
||
mozreview-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 12•7 years ago
|
||
mozreview-review-reply |
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 13•7 years ago
|
||
mozreview-review |
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 14•7 years ago
|
||
mozreview-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 15•7 years ago
|
||
mozreview-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 16•7 years ago
|
||
mozreview-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
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 18•7 years ago
|
||
mozreview-review-reply |
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?
Assignee | ||
Comment 19•7 years ago
|
||
(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 20•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 21•7 years ago
|
||
Got some test case fails, will ask for a review again after fixing the problem.
Flags: needinfo?(jyavenard)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 30•7 years ago
|
||
mozreview-review |
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 31•7 years ago
|
||
mozreview-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 32•7 years ago
|
||
mozreview-review |
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 33•7 years ago
|
||
mozreview-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 34•7 years ago
|
||
mozreview-review-reply |
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
Updated•7 years ago
|
Attachment #8906549 -
Flags: review?(matt.woodrow)
Comment 35•7 years ago
|
||
mozreview-review |
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 36•7 years ago
|
||
mozreview-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 37•7 years ago
|
||
mozreview-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 38•7 years ago
|
||
mozreview-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-
Updated•7 years ago
|
Priority: -- → P3
Comment 39•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 40•7 years ago
|
||
(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.
Comment hidden (obsolete) |
Assignee | ||
Comment 42•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 51•7 years ago
|
||
mozreview-review |
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 52•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 53•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 57•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 61•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=deefb754b5b2ac769acad98f3abce90226b4e1b5
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 62•7 years ago
|
||
Need rebase.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 71•7 years ago
|
||
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
Comment 72•7 years ago
|
||
Backed out for bustage at dom/media/ipc/VideoDecoderParent.cpp:87 (undeclared variable): https://hg.mozilla.org/integration/autoland/rev/787e7fbf47a368d4a1b7753250f541961da7af4c https://hg.mozilla.org/integration/autoland/rev/e41374eb94ec940ddbafbd04af329d7035de044a https://hg.mozilla.org/integration/autoland/rev/cfd81199cbcc71b460f07fbfe745628732811996 https://hg.mozilla.org/integration/autoland/rev/c59fd8f64b53ad036b2e7e56096ce270223f3668 https://hg.mozilla.org/integration/autoland/rev/9764ffce19aa0b20dd55032e854d65e72a1b92c1 https://hg.mozilla.org/integration/autoland/rev/d347d7618b0f9f3449274ad62d58a8cd4e381c77 https://hg.mozilla.org/integration/autoland/rev/f6eabbf84e5ecf6f800cf4e69d5951e2b25d3106 https://hg.mozilla.org/integration/autoland/rev/d1579ffe85624b4f6b0cfa3673427a538a794b7e Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f6f5863e50a8b8725bb6af0eb2cca44c561c7a6e&filter-resultStatus=runnable&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=130571511&repo=autoland > /builds/worker/workspace/build/src/dom/media/ipc/VideoDecoderParent.cpp:87:17: error: 'error' was not declared in this scope
Flags: needinfo?(alwu)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 77•7 years ago
|
||
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)
Comment 78•7 years ago
|
||
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
Comment 79•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/81e046d67cae https://hg.mozilla.org/mozilla-central/rev/f67f016f5688 https://hg.mozilla.org/mozilla-central/rev/81a7bf2ee098 https://hg.mozilla.org/mozilla-central/rev/57edd7246a04 https://hg.mozilla.org/mozilla-central/rev/5af7f08bcaab https://hg.mozilla.org/mozilla-central/rev/14249cad9e8e https://hg.mozilla.org/mozilla-central/rev/578f746a2721 https://hg.mozilla.org/mozilla-central/rev/784819df6f88
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•