Closed Bug 573865 Opened 10 years ago Closed 10 years ago

Can't build webm fallback code on Windows x86_64 (xul.dll doesn't link)

Categories

(Core :: Audio/Video, defect, minor)

x86_64
Windows Server 2008
defect
Not set
minor

Tracking

()

RESOLVED WONTFIX

People

(Reporter: neil, Assigned: neil)

Details

Attachments

(1 file, 3 obsolete files)

When building webm on Windows x86_64 without installing yasm, configure tries to use the C++ fallback code but parts of the source assume that Windows builds always have an assembler.

The first problem is that vpx_config.h should include the Windows specific headers even when not building with an assember. This is because they define FORCEINLINE correctly, which winnt.h needs in order to compile correctly.

The second problem is that several code blocks call functions that are only generated in the Windows assembler code; they need to be protected with additional ifdef guards so that they don't get used with the fallback code.
Attached patch Possible patch (obsolete) — Splinter Review
This fixes libxul builds for me. (My non-libxul build completed but wasn't actually webm-enabled, and I'm not sure why not yet.)

This might also make it possible to build 32-bit C++ fallback (without MASM) but I also haven't tried that yet.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #453233 - Flags: review?(chris.double)
Attachment #453233 - Flags: review?(chris.double) → review?(tterribe)
Comment on attachment 453233 [details] [diff] [review]
Possible patch

Tim is now the maintainer of the libvpx code I believe (previously it was Chris Pearce). I've changed to review request to him.
Comment on attachment 453233 [details] [diff] [review]
Possible patch

Any changes made to the upstream libvpx code must be placed in a separate .patch file in media/libvpx and applied in update.sh. Ideally, I would like to minimize these.

Including vpx_config_x86_64-win64-vs8.h when asm is disabled is a disaster waiting to happen, as it contains a number of #defines such as CONFIG_RUNTIME_CPU_DETECT, HAVE_MMX, etc., which obviously are incorrect. 

I think it would be better to define a generic-vs target similar to generic-gnu which does not define ARCH_X86 or ARCH_X86_64, and to get this supported by the upstream build system, so that the procedure in https://wiki.mozilla.org/WebM/Updating_libvpx can be followed to create the vpx_config files for subsequent releases. See http://www.webmproject.org/code/ for access to the upstream git repositories and for the link to the patch submission process.

This would avoid the need to introduce Mozilla-specific VPX_X86_ASM checks in upstream code entirely.
Attachment #453233 - Flags: review?(tterribe) → review-
Attached patch Proof of concept (obsolete) — Splinter Review
So as you said, providing a generic-vs8.h file means that I don't have to patch libvpx directly. I'm nowhere nearer patching webm to generate the generic-vs8.h file automatically though.
Attachment #453233 - Attachment is obsolete: true
Attachment #453506 - Flags: feedback?(tterribe)
Great. I'm willing to approve stuff here so long as you're making an effort to get the changes supported upstream. I saw your mailing list post about it. Please also update this bug with a link to the changeset when you get an upstream patch prepared.

Could you also create a new vpx_config_generic-vs8.c, and update vpx_config_c.c to include it? The changes to update.sh to copy these files over can wait until they are actually generated by the upstream libvpx, and be done the next time we import a new libvpx version.
(In reply to comment #1)
> My non-libxul build completed but wasn't actually webm-enabled
What happened here was that it started building with the wrong mozconfig, and I interrupted the build and reconfigured, but somehow nsHTMLMediaElement.cpp didn't get rebuilt. This is now working (modulo the current patch).
Attached patch Updated for review comments (obsolete) — Splinter Review
Attachment #453506 - Attachment is obsolete: true
Attachment #453569 - Flags: feedback?(tterribe)
Attachment #453506 - Flags: feedback?(tterribe)
Comment on attachment 453569 [details] [diff] [review]
Updated for review comments

>diff --git a/media/libvpx/vpx_config_c.c b/media/libvpx/vpx_config_c.c
>--- a/media/libvpx/vpx_config_c.c
>+++ b/media/libvpx/vpx_config_c.c

> #else
> /* Assume generic GNU/GCC configuration. */
>-#include "vpx_config_generic-gnu.c"
>+#include "vpx_config_generic-gnu.h"
>+#endif
> #endif
> 

This change seems to be a mistake?

>+#define CONFIG_POSTPROC 0
>+#define CONFIG_POSTPROC_GENERIC 0

Did you have to disable postprocessing entirely to get it to build? I thought disabling the ARCH_X86 stuff would have been enough to resolve all of your earlier issues. Not that this actually matters much, since I don't think we enable post-processing at all.

Otherwise this looks fine.
(In reply to comment #8)
>(From update of attachment 453569 [details] [diff] [review])
>>-#include "vpx_config_generic-gnu.c"
>>+#include "vpx_config_generic-gnu.h"
>This change seems to be a mistake?
Whoops, too much copypasta.

>>+#define CONFIG_POSTPROC 0
>>+#define CONFIG_POSTPROC_GENERIC 0
> Did you have to disable postprocessing entirely to get it to build?
No, this was actually the result of diff3 -m vpx_config_x86-win32-vs8.h vpx_config_x86-linux-gcc.h vpx_config_generic-gnu.h > vpx_config_generic-vs8.h
I misplaced an #else in the previous patch.

I also removed the configure.in error as VS8 can build generic code without ML.
Attachment #453569 - Attachment is obsolete: true
Attachment #453704 - Flags: feedback?(tterribe)
Attachment #453569 - Flags: feedback?(tterribe)
The FORCEINLINE override is apparently due to be removed; this automatically makes it possible to build the fallback code on Windows (32 or 64 bit).
As of bug 582955, yasm is now required on Win64.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Comment on attachment 453704 [details] [diff] [review]
Updated for bug fix

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

Given the status of the bug, declining the feedback request.
Attachment #453704 - Flags: feedback?(tterribe)
You need to log in before you can comment on or make changes to this bug.