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

Status: RESOLVED WONTFIX

(Core :: Audio/Video, defect)

Description

[[e-mail]]

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.

Comment 1

[[e-mail]]

Attachment: Possible patch

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.

Comment 2

[cajbir (:cajbir)]

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 3

[Timothy B. Terriberry (:derf)]

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.

Comment 4

[[e-mail]]

Attachment: Proof of concept

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.

Comment 5

[Timothy B. Terriberry (:derf)]

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.

Comment 6

[[e-mail]]

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

Comment 7

[[e-mail]]

Attachment: Updated for review comments

Comment 8

[Timothy B. Terriberry (:derf)]

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.

Comment 9

[[e-mail]]

(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

Comment 10

[[e-mail]]

Attachment: Updated for bug fix

I misplaced an #else in the previous patch.

I also removed the configure.in error as VS8 can build generic code without ML.

Comment 11

[[e-mail]]

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

Comment 12

[[e-mail]]

As of bug 582955, yasm is now required on Win64.

Comment 13

[Timothy B. Terriberry (:derf)]

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.