Stop assuming anything about result of MFStartup()

Stop assuming anything about result of MFStartup()

MFStartup() usually returns S_OK.  However, being a system library
function it doesn't have to, and we have no control over it.  It can
return an error when something goes wrong within Media Foundation, or
simply on an "N" edition of Windows that doesn't even have Media
Foundation.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2735783002
Cr-Commit-Position: refs/heads/master@{#456026}
Committed: https://chromium.googlesource.com/chromium/src/+/6d55717cd047b3d79e3cc41ea1a4e7df50b8c171

[wdzierzanowski, 2017-03-06 15:30:00]

wdzierzanowski@opera.com changed reviewers:
+ dalecurtis@chromium.org

[wdzierzanowski, 2017-03-06 15:30:00]

Hello Dale, PTAL.

[DaleCurtis, 2017-03-06 17:44:33]

DLOG_IF(ERROR, results != S_OK) ?

[wdzierzanowski, 2017-03-06 21:29:01]

Is it an "error" though? To me at least it looks more like a capability of the
operating system that may or may not be available. When I see an ERROR in the
log I read it as a condition that requires from me as a programmer some
corrective action. Here, the browser still performs all of its functions, only
switches to a different mode. For example, a failure here means that
DXVAVideoDecodeAccelerator will fail to initialize and a SW video decoder will
be used rather than GpuVideoDecoder.

[DaleCurtis, 2017-03-06 21:45:18]

Since we assume it will succeed and calling code has no idea that this may fail,
it should be reported as an error. Probably this should be changed to return
bool which can be used by the calling code if we want this not to be an error.

[wdzierzanowski, 2017-03-07 07:52:58]

I think the current assumption is that MFStartup() must be called once per
process for Media Foundation APIs to be usable, and nothing beyond that (see
MFInitializer before c3af50906850b5d9c9439edd80d996e236996c4d). Failure to call
MFStartup() or a failure within MFStartup() itself will result in failures in
the API calls later on. And we already handle API call failures.

[DaleCurtis, 2017-03-07 18:22:21]

Since this is a static though we can let code call this at any call site which
needs this functionality and handle the error code if we want. Without looking
at all MF users I don't know how viable that is.

As such I agree that this shouldn't be a DCHECK() since it's a handled failure
case, given that functionality may still work, what do you think about DLOG,
WARNING?

[wdzierzanowski, 2017-03-07 18:57:21]

On 2017/03/07 18:22:21, DaleCurtis wrote:
> Since this is a static though we can let code call this at any call site which
> needs this functionality and handle the error code if we want. Without looking
> at all MF users I don't know how viable that is.

InitializeMediaFoundation() never returned a value, so I'd say no current MF
users need the error code. As mentioned earlier, a failure in MFStartup()
triggers the normal error code paths in MF users when they try to call any MF
API.

> As such I agree that this shouldn't be a DCHECK() since it's a handled failure
> case, given that functionality may still work, what do you think about DLOG,
> WARNING?

I think WARNING is better than ERROR. But personally I'd treat this as an
informational message only. For example, for someone running the browser on an
"N" edition of Windows there is nothing to warn about as Media Foundation is
simply not there by definition.

A warning is more appropriate when Media Foundation actually tries to initialize
and fails. We could try to tell these two cases apart by checking for the
E_NOTIMPL value, but I'm not sure if this is not starting to get overly
complicated for the benefits it provides.

[DaleCurtis, 2017-03-07 19:16:21]

On 2017/03/07 at 18:57:21, wdzierzanowski wrote:
> On 2017/03/07 18:22:21, DaleCurtis wrote:
> > Since this is a static though we can let code call this at any call site
which
> > needs this functionality and handle the error code if we want. Without
looking
> > at all MF users I don't know how viable that is.
> 
> InitializeMediaFoundation() never returned a value, so I'd say no current MF
users need the error code. As mentioned earlier, a failure in MFStartup()
triggers the normal error code paths in MF users when they try to call any MF
API.

Yes, but typically we don't rely on cascading or implicit failures in Chrome.
We'd instead prefer such failure was explicit and its purpose understandable. So
changing this method to return a bool status code and ensure call sites use that
to opt-out of MF APIs is preferable. We've gone from many potential failure
sites where each writer needs to understand that MF may be unavailable (not a
widely known fact), to a single call site which can have a solid comment
explaining why the return value of this function must be checked.

It looks like there are only 3 callers to this function, so I recommend changing
it to a bool and triggering the error paths in those respective call sites with
more context appropriate warnings or no warning at all if we deem that
necessary.

> 
> > As such I agree that this shouldn't be a DCHECK() since it's a handled
failure
> > case, given that functionality may still work, what do you think about DLOG,
> > WARNING?
> 
> I think WARNING is better than ERROR. But personally I'd treat this as an
informational message only. For example, for someone running the browser on an
"N" edition of Windows there is nothing to warn about as Media Foundation is
simply not there by definition.
> 
> A warning is more appropriate when Media Foundation actually tries to
initialize and fails. We could try to tell these two cases apart by checking for
the E_NOTIMPL value, but I'm not sure if this is not starting to get overly
complicated for the benefits it provides.

[wdzierzanowski, 2017-03-07 19:28:18]

Makes sense. It'll now be a small refactoring task rather than a trivial bug
fix, but I can do that.

[wdzierzanowski, 2017-03-08 08:16:05]

Description was changed from

==========
Stop assuming anything about result of MFStartup()

MFStartup() usually returns S_OK.  However, being a system library
function it doesn't have to, and we have no control over it.  It can
return an error when something goes wrong within Media Foundation, or
simply on an "N" edition of Windows that doesn't even have Media
Foundation.
==========

to

==========
Stop assuming anything about result of MFStartup()

MFStartup() usually returns S_OK.  However, being a system library
function it doesn't have to, and we have no control over it.  It can
return an error when something goes wrong within Media Foundation, or
simply on an "N" edition of Windows that doesn't even have Media
Foundation.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[wdzierzanowski, 2017-03-08 08:17:09]

Done in patch set 2.

[DaleCurtis, 2017-03-09 20:07:31]

lgtm, thanks for bearing with me!

[DaleCurtis, 2017-03-09 20:08:14]

https://codereview.chromium.org/2735783002/diff/20001/media/base/win/mf_initi...
File media/base/win/mf_initializer.h (right):

https://codereview.chromium.org/2735783002/diff/20001/media/base/win/mf_initi...
media/base/win/mf_initializer.h:12: // Makes sure MFStartup() is called exactly
once.  Returns true if Media
Oh one thing, it'd be nice to explain here why this can fail. I.e. N version of
Windows; perhaps even linking to the wiki entry.

[wdzierzanowski, 2017-03-09 22:14:36]

https://codereview.chromium.org/2735783002/diff/20001/media/base/win/mf_initi...
File media/base/win/mf_initializer.h (right):

https://codereview.chromium.org/2735783002/diff/20001/media/base/win/mf_initi...
media/base/win/mf_initializer.h:12: // Makes sure MFStartup() is called exactly
once.  Returns true if Media
On 2017/03/09 20:08:14, DaleCurtis wrote:
> Oh one thing, it'd be nice to explain here why this can fail. I.e. N version
of
> Windows; perhaps even linking to the wiki entry.

Done.

[wdzierzanowski, 2017-03-10 07:40:09]

Thanks for reviewing!

[wdzierzanowski, 2017-03-10 07:40:24]

The CQ bit was checked by wdzierzanowski@opera.com

[wdzierzanowski, 2017-03-10 07:40:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2735783002/#ps40001
(title: "Mention "N" edition")

[commit-bot: I haz the power, 2017-03-10 07:40:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 09:05:31]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489131624379750,
"parent_rev": "83749ddd60c44dfc2c418545ae47e62b686e0cd9", "commit_rev":
"6d55717cd047b3d79e3cc41ea1a4e7df50b8c171"}

[commit-bot: I haz the power, 2017-03-10 09:06:12]

Description was changed from

==========
Stop assuming anything about result of MFStartup()

MFStartup() usually returns S_OK.  However, being a system library
function it doesn't have to, and we have no control over it.  It can
return an error when something goes wrong within Media Foundation, or
simply on an "N" edition of Windows that doesn't even have Media
Foundation.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Stop assuming anything about result of MFStartup()

MFStartup() usually returns S_OK.  However, being a system library
function it doesn't have to, and we have no control over it.  It can
return an error when something goes wrong within Media Foundation, or
simply on an "N" edition of Windows that doesn't even have Media
Foundation.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2735783002
Cr-Commit-Position: refs/heads/master@{#456026}
Committed:
https://chromium.googlesource.com/chromium/src/+/6d55717cd047b3d79e3cc41ea1a4...
==========

[commit-bot: I haz the power, 2017-03-10 09:06:18]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/6d55717cd047b3d79e3cc41ea1a4...