682215 - Memory reporters for VPX-related allocations

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

+++ This bug was initially created as a clone of Bug #677653 +++

See bug 677653 comment 6 for a lead on how do this for libvpx.

Comment 1

[Nathan Froyd [:froydnj]]

This doesn't really depend on any of the vorbis stuff in bug 677653 landing.  But it does depend on figuring out how to intelligently export the libvpx header files.  Trying |#include "vpx/vpx_mem.h"| from anywhere not in media/libvpx/ gets you:

In file included from /home/froydnj/src/gecko-dev.git/xpcom/build/nsXPComInit.cpp:133:0,
                 from /opt/build/froydnj/build-mc/xpcom/build/Unified_cpp_xpcom_build2.cpp:67:
../../dist/include/vpx/vpx_mem.h:15:24: fatal error: vpx_config.h: No such file or directory
compilation terminated.

I guess we also want to export vpx_config.h and all its subheaders?  Though it looks like vpx_config.h isn't really set up to provide configuration in a platform-generic manner, e.g. you'll only get a correct configuration for Windows if VPX_X86_ASM is defined, which seems unusual.

Comment 2

[Ralph Giles (:rillian)]

I guess we need to export vpx_config* if we want the other headers to be useful.

Alternatively, you could LOCAL_INCUDES += ['/media/libvpx'] and #include <vpx_mem/vpx_mem.h> instead. Or put the memory reporter in the libvpx tree.

> Though it looks like vpx_config.h isn't really set up to provide configuration in a platform-generic manner

This is all a hack because mach doesn't support calling external build systems. Rather than maintain a completely parallel build description, our import script runs the upstream configure for each target, copies that into our tree as a platform-specific header, and then includes the appropriate one from a vpx_config.h wrapper.

IIRC the asm-platform inversion is because we don't have a platform CPP define for android, and the platform-specific headers assume asm is available. Not saying it can't be cleaned up, of course.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Ralph Giles (:rillian) from comment #2)
> Alternatively, you could LOCAL_INCUDES += ['/media/libvpx'] and #include
> <vpx_mem/vpx_mem.h> instead. Or put the memory reporter in the libvpx tree.

Putting the memory reporter in the libvpx tree defeats the purpose of having vpx_mem_set_functions. :)  The LOCAL_INCLUDES thing seems plausible, I guess; xpcom/build already pulls in a bunch of different directories, what's one more?

> > Though it looks like vpx_config.h isn't really set up to provide configuration in a platform-generic manner
> 
> IIRC the asm-platform inversion is because we don't have a platform CPP
> define for android, and the platform-specific headers assume asm is
> available. Not saying it can't be cleaned up, of course.

__ANDROID__ doesn't work?

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: add a memory reporter for memory from libvpx

This patch is based on top of the patches in bug 677653 for convenience.

Nick for the memory reporting bits, Ted for the build bits, and Benjamin to
confirm that we're OK calling out to libogg and libvpx from xpcom/build/ (which
I guess should really cover the patches in bug 677653 too..).

Comment 5

[Benjamin Smedberg]

Comment on attachment 8382244 [details] [diff] [review]
add a memory reporter for memory from libvpx

What library is vpx_mem_set_functions in? If gkmedias.dll will it hurt startup speed to load that here instead of later when we actually need it? Otherwise this seems fine.

Also does this need a feature ifdef? Are there configurations that don't have vpx?

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> Comment on attachment 8382244 [details] [diff] [review]
> add a memory reporter for memory from libvpx
> 
> What library is vpx_mem_set_functions in? If gkmedias.dll will it hurt
> startup speed to load that here instead of later when we actually need it?
> Otherwise this seems fine.

Yes, it's in gkmedias.  I haven't tested to see whether it will affect startup speed.

I don't know whether there's a good place for the vpx_mem_set_functions call to ensure that it happens prior to any libvpx memory allocation.  Perhaps someplace in content/media/?  ni?'ing :cpearce for that.

> Also does this need a feature ifdef? Are there configurations that don't
> have vpx?

Doh, yes.  I did the MOZ_VPX dance for the #include, but failed to do it for everything else. =/

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8382244 [details] [diff] [review]
add a memory reporter for memory from libvpx

Experimentation with the ogg patch suggests that even asking XPCOM to access gkmedias this early is causing problems.  Canceling reviews for now until things get sorted.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

I'm no expert on libvpx, but I'd guess we'd need to set memory reporters before all calls to vpx_codec_dec_init or vpx_codec_vp8_dx() or both... Note we use libvpx inside the WebRTC code too.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: add a memory reporter for memory from libvpx

OK, the gkmedias bits have been sorted out along with the MOZ_VPX bits.  This
patch is ready for review.  It applies on top of the patches in bug 677653.

Nick for the memory reporting bits, Ted for the moz.build bits.

Comment 10

[Nathan Froyd [:froydnj]]

Landed with a simple change to symbols.def.in so vpx_mem_set_functions is visible outside gkmedias.dll:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3719528f8252

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3719528f8252