Closed
Bug 1396547
Opened 7 years ago
Closed 7 years ago
Use Supports() in PDMFactory::CreateDecoder() instread of SupportsMimeType()
Categories
(Core :: Audio/Video: Playback, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: alwu, Assigned: alwu)
References
Details
Attachments
(1 file)
The TrackInfo [1] created in WMFDecoderModule::SupportsMimeType() doesn't contain valid image's width and height, because the TrackInfo is created without width and height [2] and the default width and height are both -1 [3]. Thesefore, we can't correctly judge whether this resolution is supported by MFT. [4]. We should use Supports() instead of SupportsMimeType(). [1] https://goo.gl/QV8Jgm [2] https://goo.gl/4siShn [3] https://goo.gl/BDoXYf [4] https://goo.gl/BZh4QA
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
And how would the resolution help anyway? The only place it matters is for h264 as that's where the H264 MFT has restrictions. But there's no other decoders that can do h264. so we would fail just the same, just in a different place. Except that following this bug, the information given back is that we don't handle that mimetype (unsupported content), as opposed to: we handle the mimetype, but can't support this resolution (information passed back to the decoder doctor).
Updated•7 years ago
|
Flags: needinfo?(alwu)
Assignee | ||
Comment 3•7 years ago
|
||
Yes, I'm targeting the h264 here. The code [1] is used to check whether the video's resolution is supported by MFT, and I think we should immediately return false if the resolution is not support, instead of waiting the decode error. In current code, the |videoInfo->mImage.width| and |videoInfo->mImage.height| would always be -1, so the WMFDecoderModule::Supports() would return true even the resolution is not supported. [1] http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/dom/media/platforms/wmf/WMFDecoderModule.cpp#240-259
Flags: needinfo?(alwu)
Comment 4•7 years ago
|
||
Yes, but by doing so you will no longer allow the DecoderDoctor to report on why playback failed. It will only show that the media isn't supported.
Updated•7 years ago
|
Attachment #8904215 -
Flags: review?(cpearce) → review?(jyavenard)
Comment 5•7 years ago
|
||
I'll pass the review here to jya, as he knows this code better than I.
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #4) > Yes, but by doing so you will no longer allow the DecoderDoctor to report on > why playback failed. > It will only show that the media isn't supported. First, from MFT's output [1], it doesn't have the specific returing value which can cleary tell us the error is due to the unsupported resolution. Therefore, we should handle this kind of error by ourselves. However, in current code, - If the resolution is above the maximum, the checking doesn't work as I said in comment3. - If the resolution is under the minimum, we couldn't create the decoder [2] because the checking failed [3] Both cases we didn't send the correct error for decoder doctor. [1] https://msdn.microsoft.com/zh-tw/library/windows/desktop/ms704014(v=vs.85).aspx [2] http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/dom/media/platforms/wmf/WMFDecoderModule.cpp#114 [3] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/dom/media/platforms/wmf/WMFVideoMFTManager.cpp#543 --- In addition, the resolution checking is used on two different places, I think we should keep all logic in one place, the WMFDecoderModule::Supports() seems the perfect place. If we don't support this resolution, we don't need to waste time to create decoder and do other related steps. If you want to show the correspond error, we could show it on the console, like [4]. [4] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/dom/media/platforms/wmf/WMFVideoMFTManager.cpp#538
Flags: needinfo?(jyavenard)
Comment hidden (obsolete) |
Assignee | ||
Comment 8•7 years ago
|
||
Hmm, I knew what you said now. Maybe we should move all resolution check into MFTManager::Init() and then call that in MediaDataDecoder::Init(), we would return error and correct description if the resolution is unsupported.
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8904215 [details] Bug 1396547 - use Supports() in PDMFactory::CreateDecoder() instread of SupportsMimeType(). https://reviewboard.mozilla.org/r/175994/#review181244 That's okay by me, however we need a way to notify the user on why it failed to find a decoder. Without this patch, we would have allowed to create a h264 decoder, as such PDMFactory::CreateDecoder would have succeeded. It's only later that the init would have failed. At this level, we can provide more details on why it failed (here, the resolution is invalid). If it's now PDMFactory::CreateDecoder() that failed to create a decoder, the error will now be just about unsupported mimetype. So please create a new bug to provide better error reporting / diagnostic when we fail to create a decoder. thank you
Attachment #8904215 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #10) > Comment on attachment 8904215 [details] > Bug 1396547 - use Supports() in PDMFactory::CreateDecoder() instread of > SupportsMimeType(). > > https://reviewboard.mozilla.org/r/175994/#review181244 > > That's okay by me, however we need a way to notify the user on why it failed > to find a decoder. > Without this patch, we would have allowed to create a h264 decoder, as such > PDMFactory::CreateDecoder would have succeeded. It's only later that the > init would have failed. At this level, we can provide more details on why it > failed (here, the resolution is invalid). > If it's now PDMFactory::CreateDecoder() that failed to create a decoder, the > error will now be just about unsupported mimetype. Actually, thats is exactly present situation (even without this patch). Since WMFVideoManager::init() would fail so that we can't create the decoder and won't show the correct error description on the console. > So please create a new bug to provide better error reporting / diagnostic > when we fail to create a decoder. > > thank you Yes, I'll file new bug to handle this issue. Thanks!
Assignee | ||
Comment 12•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fce6ca73d4ddad2f2ee23cc9a65cb2d0eb8d984&selectedJob=128264965
Comment 13•7 years ago
|
||
Pushed by alwu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9f16ba6aac28 use Supports() in PDMFactory::CreateDecoder() instread of SupportsMimeType(). r=jya
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9f16ba6aac28
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
•