Closed Bug 1396547 Opened 4 years ago Closed 4 years ago

Use Supports() in PDMFactory::CreateDecoder() instread of SupportsMimeType()

Categories

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

Other Branch
enhancement
Not set
normal

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
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).
Flags: needinfo?(alwu)
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)
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.
Attachment #8904215 - Flags: review?(cpearce) → review?(jyavenard)
I'll pass the review here to jya, as he knows this code better than I.
(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)
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.
Will upload new patch tomorrow.
Flags: needinfo?(jyavenard)
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+
(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!
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9f16ba6aac28
use Supports() in PDMFactory::CreateDecoder() instread of SupportsMimeType(). r=jya
Blocks: 1397141
https://hg.mozilla.org/mozilla-central/rev/9f16ba6aac28
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.