Closed Bug 823921 Opened 7 years ago Closed 7 years ago

Fix WMF backend compilation on mingw

Categories

(Core :: Audio/Video, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: jacek, Assigned: jacek)

Details

Attachments

(3 files)

Attached patch linkage fixSplinter Review
The attached patch uses linker options for specifying linked libraries.

It also undefined GetCurrentTime in MediaDecoder.h. That part is not really mingw-dependent. It's a problem with a macro in windows headers and it's an accident that mingw trigers it. MSVC could just as well, depending on headers include orider. We already do this in a few places across the tree.
Attachment #694833 - Flags: review?(khuey)
mingw has limited support for _com_ptr_t (aka. _COM_SMARTPTR_TYPEDEF). I started looking at fixing it there, but realized that it would be better (also for Mozilla) to fix it here. _com_ptr_t handles errors using exceptions, which are not used in Mozilla code. The attached patch replaces usage of _com_ptr_t by RefPtr, like most other code that deals with MS COM.
Attachment #694835 - Flags: review?(cpearce)
Attached patch warning fixesSplinter Review
Warning fixes. As separate patch for easier review and separation of less important part.
Assignee: nobody → jacek
Attachment #694836 - Flags: review?(paul)
Comment on attachment 694835 [details] [diff] [review]
Use RefPtr instead of _com_ptr_t

I forgot to mention, this is green on try: https://tbpl.mozilla.org/?tree=Try&rev=b53b4aa772a5
Attachment #694835 - Flags: review?(cpearce) → review?(paul)
Comment on attachment 694836 [details] [diff] [review]
warning fixes

Review of attachment 694836 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #694836 - Flags: review?(paul) → review+
Comment on attachment 694835 [details] [diff] [review]
Use RefPtr instead of _com_ptr_t

Review of attachment 694835 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this aligns better with other Windows specific code we have in the tree (mainly D2D draw target, from what I read).

Thanks.

::: content/media/wmf/WMFByteStream.cpp
@@ +180,2 @@
>    NS_ENSURE_TRUE(SUCCEEDED(hr), hr);
> +  hr = unknown->QueryInterface((IMFAsyncResult**)byRef(callerResult));

Can you |static_cast<IMFAsyncResult**>(byRef(callerResult))| here instead?

::: content/media/wmf/WMFReader.cpp
@@ +512,5 @@
>    // but only some systems (Windows 8?) support it.
>    BYTE* data = nullptr;
>    LONG stride = 0;
> +  RefPtr<IMF2DBuffer> twoDBuffer;
> +  hr = buffer->QueryInterface((IMF2DBuffer**)byRef(twoDBuffer));

Same thing, prefer C++ style casts. static_cast<> also works in this case.
Attachment #694835 - Flags: review?(paul) → review+
Attachment #694836 - Flags: checkin+
Thanks for the review, pushed a version with static_casts:

https://hg.mozilla.org/integration/mozilla-inbound/rev/158a9c8668bb
Attachment #694835 - Flags: checkin+
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/21d5fbda37b8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(In reply to Jacek Caban from comment #3)
> Comment on attachment 694835 [details] [diff] [review]
> Use RefPtr instead of _com_ptr_t
> 
> I forgot to mention, this is green on try:
> https://tbpl.mozilla.org/?tree=Try&rev=b53b4aa772a5

Note that the WMF backend is currently disabled on m-c by default, so unless you toggle the pref in modules/libpref/src/init/all.js, WMF won't actually be tested... And currently the tests still randomly fail for WMF anyway...

BTW, we're still using _com_ptr_t in webrtc:
http://mxr.mozilla.org/mozilla-central/search?string=_COM_SMARTPTR_TYPEDEF
(I wrote at least some of that code too, sorry!)
You need to log in before you can comment on or make changes to this bug.