Closed Bug 823921 Opened 7 years ago Closed 7 years ago

Fix WMF backend compilation on mingw


(Core :: Audio/Video, defect)

Windows 7
Not set





(Reporter: jacek, Assigned: jacek)



(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:
Attachment #694835 - Flags: review?(cpearce) → review?(paul)
Comment on attachment 694836 [details] [diff] [review]
warning fixes

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

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).


::: content/media/wmf/WMFByteStream.cpp
@@ +180,2 @@
> +  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:
Attachment #694835 - Flags: checkin+
Whiteboard: [leave open]
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:

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:
(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.