677653 - Memory reporters for Ogg-related allocations

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Jesse Ruderman]

Attachment: example about:memory

Nightly - 8.0a1 (2011-08-09)      

1. Go to http://www.thewildernessdowntown.com/
2. Type a street address (I used one in Mountain View)
3. Open about:memory in another tab.

Result: Heap-unclassified jumps
  from   ~ 68.47 MB (23.70%)
  to     ~ 267.55 MB (45.55%)

Comment 1

[Nicholas Nethercote [inactive]]

I'll get to this with DMD at some point.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: DMD output

I only see about 31% heap-unclassified at that point, perhaps things have improved since then.

I attach the about:memory + DMD output for the top 10 records.  The most interesting part is that about:memory says this:

├──113,231,616 B (62.19%) -- media
│  ├──112,869,632 B (61.99%) -- decoded-video
│  └──────361,984 B (00.20%) -- decoded-audio

And DMD's first entry is this:

==2652== Unreported: 152,780,800 (cumulative: 152,780,800) bytes in 166 heap block(s) in record 1 of 10858:
==2652==  Requested bytes unreported: 112,869,632 / 112,869,632
==2652==  Slop      bytes unreported: 39,911,168 / 39,911,168
==2652==    at 0x4029F90: malloc (vg_replace_malloc.c:235)
==2652==    by 0x403B04B: moz_xmalloc (mozalloc.cpp:110)
==2652==    by 0x79236C6: mozilla::layers::BasicPlanarYCbCrImage::SetData(mozilla::layers::PlanarYCbCrImage::Data c
onst&) (mozalloc.h:241)
==2652==    by 0x7023CEB: VideoData::Create(nsVideoInfo&, mozilla::layers::ImageContainer*, long, long, long, Video
Data::YCbCrBuffer const&, int, long, nsIntRect) (nsBuiltinDecoderReader.cpp:186)
==2652==    by 0x7039A0E: nsOggReader::DecodeTheora(ogg_packet*, long) (nsOggReader.cpp:480)
==2652==    by 0x7039BE8: nsOggReader::DecodeVideoFrame(int&, long) (nsOggReader.cpp:525)

Now, I haven't taught DMD about "media/decoded-video" but the number 112,869,632 appears in both outputs, so looks like media/decoded-video is covering the requested parts of these allocations perfectly, but is missing the slop completely.  This is exactly what we saw previously in bug 680358.

But there's another 35MB or so of memory that isn't being accounted for in other Theora-related memory, shown in the subsequent records.

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> 
> But there's another 35MB or so of memory that isn't being accounted for in
> other Theora-related memory, shown in the subsequent records.

I'm going to re-title this bug to be about that.

kinetik, I've assigned it to you because you did bug 672755, please change if that's not appropriate.

Comment 4

[Nicholas Nethercote [inactive]]

BTW, using moz_malloc_usable_size to measure the memory use (with a fallback if it returns 0, for platforms that don't support it) would be a good idea :)

Comment 5

[Matthew Gregan [:kinetik]]

We should do this, and report the same for VPX and Vorbis.  I don't think any of them expose APIs that provide this information, though, and it feels somewhat wrong to add APIs for this purpose.

CCing derf in the hope that he knows a good way to expose this info.

Comment 6

[Timothy B. Terriberry (:derf)]

libogg, libvorbis, libtremor, and libtheora run all their allocations through _ogg_malloc, _ogg_calloc, _ogg_realloc, and _ogg_free macros (defined in os_types.h in libogg). Those could be overridden to track allocations made by these libraries, potentially with a different override for each library. This can be done entirely in the build system. Unfortuantely, it looks like os_types.h unconditionally defines these macros to map to the standard libc functions without checking to see if they're already defined, but that's easy enough to fix.

libvpx also has a memory tracking infrastructure in vpx_mem/vpx_mem.[ch], but I'm less clear on the details of what's available. There's both vpx_malloc and friends and duck_malloc and friends. I think the latter just always gets defined to the vpx_ version. These are not macros (unless libvpx's internal memory tracking is enabled), but there are global function pointers that can be used to override them (see vpx_mem_set_functions).

That should be enough to globally track allocations from these libraries, which is all the current media memory reporters do. If in the future you want to tie them to a particular tab, one approach would be to use global thread-local storage to save the address of the memory reporter you want to tie allocations to before calling any of those library functions, and then look that up your overridden allocator.

Comment 7

[Nicholas Nethercote [inactive]]

There are two basic ways to implement memory reporters: "maintain a counter" and "traverse a data structure".  The latter is preferable, see bug 570785 comment 5 for a little more explanation.

But if that's hard to do, you could take a look at bug 669116 for inspiration on implementing a "maintain a counter"-style reporter by wrapping library-specific allocation functions.

Comment 8

[Timothy B. Terriberry (:derf)]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> There are two basic ways to implement memory reporters: "maintain a counter"
> and "traverse a data structure".  The latter is preferable, see bug 570785
> comment 5 for a little more explanation.

Yeah, the problem is that most of the memory is allocated in data structures that are not part of the public API of these libraries, so in order to get code that has access to the definitions of these data structures, it'd have to be in the libraries themselves, and that's much more invasive (requiring modifying the data structures themselves, as they often don't store the size of their allocations, at least not directly). On the plus side of using the counter approach, if there _is_ an error and the numbers do become bogus, it almost certainly means there's a bug in these libraries themselves (if nothing more than accidentally using malloc instead of _ogg_malloc somewhere, but still a bug), so that's interesting in its own right.

Comment 9

[Nicholas Nethercote [inactive]]

Yes, for libraries of that nature "maintain a counter" is probably best.

Comment 10

[Matthew Gregan [:kinetik]]

I'll install separate _ogg_* allocators hooks for each of Ogg, Theora, Vorbis, and Tremor.

Comment 11

[Nicholas Nethercote [inactive]]

kinetik, is this on your radar at the moment?

Comment 12

[Matthew Gregan [:kinetik]]

I'll try to get to it in the next couple of weeks.  If anyone wants to grab it off of me before then, feel free!

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Timothy B. Terriberry (:derf) from comment #6)
> libogg, libvorbis, libtremor, and libtheora run all their allocations
> through _ogg_malloc, _ogg_calloc, _ogg_realloc, and _ogg_free macros
> (defined in os_types.h in libogg). Those could be overridden to track
> allocations made by these libraries, potentially with a different override
> for each library. This can be done entirely in the build system.
> Unfortuantely, it looks like os_types.h unconditionally defines these macros
> to map to the standard libc functions without checking to see if they're
> already defined, but that's easy enough to fix.

Adding some #ifdeffery to fix that is easy enough, but I'm less clear on how to ensure that all includers of os_types.h see the same definition of the macros.  Do you think it's worth just patching os_types.h in update.sh or similar to use our custom malloc functions?

> libvpx also has a memory tracking infrastructure in vpx_mem/vpx_mem.[ch],
> but I'm less clear on the details of what's available. There's both
> vpx_malloc and friends and duck_malloc and friends. I think the latter just
> always gets defined to the vpx_ version. These are not macros (unless
> libvpx's internal memory tracking is enabled), but there are global function
> pointers that can be used to override them (see vpx_mem_set_functions).

Same sort of issue applies here, except we would need to find a good place to call vpx_mem_set_functions prior to any vpx_* functions being called.  Any pointers on where that should be?  I guess we could do it in some before-xpcom-and-the-world-starts function, but I'm unsure of what that would do to startup time.

Comment 14

[Timothy B. Terriberry (:derf)]

(In reply to Nathan Froyd (:froydnj) from comment #13)
> macros.  Do you think it's worth just patching os_types.h in update.sh or
> similar to use our custom malloc functions?

That is also a reasonable approach.

> Same sort of issue applies here, except we would need to find a good place
> to call vpx_mem_set_functions prior to any vpx_* functions being called. 
> Any pointers on where that should be?  I guess we could do it in some
> before-xpcom-and-the-world-starts function, but I'm unsure of what that
> would do to startup time.

libvpx itself has a series of such functions which must be called to initialize the run-time CPU detection infrastructure (e.g., vp8_rtcd() and friends). Their approach is to use pthread_once() invocations (with a portable wrapper for Windows) inside the decoder and encoder initialization functions (for both VP8 and VP9).

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to Timothy B. Terriberry (:derf) from comment #14)
> > Same sort of issue applies here, except we would need to find a good place
> > to call vpx_mem_set_functions prior to any vpx_* functions being called. 
> > Any pointers on where that should be?  I guess we could do it in some
> > before-xpcom-and-the-world-starts function, but I'm unsure of what that
> > would do to startup time.
> 
> libvpx itself has a series of such functions which must be called to
> initialize the run-time CPU detection infrastructure (e.g., vp8_rtcd() and
> friends). Their approach is to use pthread_once() invocations (with a
> portable wrapper for Windows) inside the decoder and encoder initialization
> functions (for both VP8 and VP9).

Hm, I was hoping to not have to modify libvpx itself, but that might be the best way to make sure we're using the correct bits.

Comment 16

[Timothy B. Terriberry (:derf)]

(In reply to Nathan Froyd (:froydnj) from comment #15)
> Hm, I was hoping to not have to modify libvpx itself, but that might be the
> best way to make sure we're using the correct bits.

Depends on how airtight you want to make things. Duplicating the once() functionality in a couple of wrapper functions would at least protect you so long as you include the header that defines the wrapper (but not against someone who includes the libvpx headers directly). Doing something early in startup has its flaws (besides performance), too.

Comment 17

[Nathan Froyd [:froydnj]]

So I took a look at this today, just for libogg; libvpx comes later.  The strategy adopted is:

1. Modify libogg/os_types.h to do an #ifndef/#define/#endif dance.

2. Modify mozilla-config.h.in to do something like:

#if MOZ_OGG && defined(MOZ_DIRECTORY_USES_OGG)
#include "ogg_alloc_hooks.h"
#else
/* Define _ogg_* to something that induces build errors, e.g.: */
extern void you_need_to_define_MOZ_DIRECTORY_USES_OGG(void);

#define _ogg_malloc you_need_to_define_MOZ_DIRECTORY_USES_OGG
/* ...and so forth... */
#endif

ogg_alloc_hooks.h defines _ogg_* in the obvious way.

3. Add MOZ_DIRECTORY_USES_OGG where appropriate.

4. Export an ogg_alloc_hooks.h and add out-of-line implementations of the ogg allocation wrappers.

5. Build.

6. Profit.

This is similar in spirit to what was done for hunspell in bug 669116, but a little more wide-ranging because more things use libogg.

I realize that steps 2 and 3 are not the cleanest, but I'm not sure there's a better way.  The alternative I see is to not export the ogg_alloc_hooks.h and instead require the appropriate directories to add something to LOCAL_INCLUDES.  This seemed less attractive because _ogg_malloc et al. mismatches become harder to detect.

Unfortunately, I haven't been to get to step 6 yet because I've defined the out-of-line allocation wrappers in content/media/, and those wrappers don't get included in the link for webrtc tests that link against gkmedias.  Sticking the out-of-line wrappers in libogg itself would be one way out of this, but I was hoping to avoid touching libogg more than I had to.

Another way out might be to not have out-of-line wrappers and instead define everything inline in ogg_alloc_hooks.h.  I wasn't sure that could be made to work well, given that we're going to use that header in both C and C++ code, and I'd like to not have a bunch of little copies of the wrapper floating around everywhere.  (Anyway, that only pushes the problem back, because the eventual goal is to have those wrappers count allocations anyway, and the counting functions and/or the counter itself needs to live out-of-line somewhere.)

glandium, you're going to wind up reviewing chunks of this anyway; do you have suggestions on how to modify the above strategy to deal with webrtc tests?

Comment 18

[Mike Hommey [:glandium]]

Ideally, libraries would allow to setup hooks at runtime, and we'd just register them.

Comment 19

[Nathan Froyd [:froydnj]]

Attachment: part 1 - indirect libogg memory allocations through variables

Here's a different approach: all problems in programming can be solved with
another layer of indirection.  Instead of #define'ing separate functions, let's
just #define variables that can be set at runtime.  Maybe we want some other
way of redefining things, but the variables themselves need to live in libogg
so that webrtc links OK.

Not sure who the right person to comment on this is, feel free to bounce
feedback around as necessary.

Comment 20

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add a memory reporter for vorbis-related allocations

Now that we have hooks into libogg, we can just set up our own memory reporter
for that memory.

I'm not completely sure that we want a counting reporter here.  Since we can
ensure that all libogg (and related) memory is flowing through our allocator
now, can we just start counting the relevant buffers in the audio/video memory
reporters instead?  That would give better numbers, DMD-wise.  Or do we already
count those and this stuff is just for internal library state?  f?'ing Ehsan as
a potential candidate to answer those questions, feel free to redirect.

Comment 21

[(no longer active)]

Comment on attachment 8381479 [details] [diff] [review]
part 2 - add a memory reporter for vorbis-related allocations

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

Chris is a better person to provide feedback here.

Comment 22

[Ralph Giles (:rillian)]

Comment on attachment 8381477 [details] [diff] [review]
part 1 - indirect libogg memory allocations through variables

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

Yep, that's what the _ogg_malloc hooks are for.

Please include a copy of the changes as a patch file in-tree (not including moz.build), and apply it at the end of media/libogg/update.sh so we don't clobber this when pulling new upstream releases.

Comment 23

[Ralph Giles (:rillian)]

Note that hooking ogg_malloc &c. will also capture libtheora allocation traffic, so 'VorbisReporter' isn't an accurate name.

Comment 24

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

Comment on attachment 8381479 [details] [diff] [review]
part 2 - add a memory reporter for vorbis-related allocations

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

What Ralph said; VorbisReporter isn't a good name. OggReporter maybe?

Comment 25

[Nicholas Nethercote [inactive]]

Comment on attachment 8381479 [details] [diff] [review]
part 2 - add a memory reporter for vorbis-related allocations

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

I'll raise your f? with an r+.

::: xpcom/build/nsXPComInit.cpp
@@ +404,5 @@
>  NS_IMPL_ISUPPORTS1(ICUReporter, nsIMemoryReporter)
>  
>  /* static */ Atomic<size_t> ICUReporter::sAmount;
>  
> +class VorbisReporter MOZ_FINAL : public nsIMemoryReporter

Rename the class to match the path.

@@ +468,5 @@
> +    CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData)
> +    {
> +        return MOZ_COLLECT_REPORT(
> +            "explicit/ogg", KIND_HEAP, UNITS_BYTES, sAmount,
> +            "Memory allocated through libogg for Vorbis, Ogg, Opus, and related media files.");

Elsewhere we have "explicit/media/decoded-video" and
"explicit/media/decoded-audio". Would "explicit/media/libogg" be a better path?

@@ +683,5 @@
>          mozilla::SystemMemoryReporter::Init();
>      }
>  
>      // The memory reporter manager is up and running -- register a reporter for
> +    // ICU's and Vorbis's memory usage.

s/Vorbis/libogg/

Comment 26

[Nicholas Nethercote [inactive]]

> I'm not completely sure that we want a counting reporter here.  Since we can
> ensure that all libogg (and related) memory is flowing through our allocator
> now, can we just start counting the relevant buffers in the audio/video
> memory reporters instead?  That would give better numbers, DMD-wise.

Counting reporters integrate nicely with DMD so long as you do the counting with functions defined with 
MOZ_DEFINE_MALLOC_SIZE_OF_ON_ALLOC and MOZ_DEFINE_MALLOC_SIZE_OF_ON_FREE, as this patch does.

Comment 27

[Nathan Froyd [:froydnj]]

Attachment: part 1 - indirect libogg memory allocations through variables

This time with the added patch file and update.sh changes.

Comment 28

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add a memory reporter for vorbis-related allocations

Nick f+'d the patch, but commented that it would be r+, so I'm going to assume
that he's OK with the following changes and just make this r+:

- renamed VorbisReporter to LiboggReporter;
- tweaked description accordingly;
- made the reporting path explicit/media/libogg.

Comment 29

[Ralph Giles (:rillian)]

Comment on attachment 8382240 [details] [diff] [review]
part 1 - indirect libogg memory allocations through variables

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

r=me with the patch nit addressed. Thanks for hooking us up!

::: media/libogg/update.sh
@@ +11,5 @@
>  cp $1/src/bitwise.c ./src/ogg_bitwise.c
>  cp $1/src/framing.c ./src/ogg_framing.c
>  cp $1/AUTHORS ./AUTHORS
>  patch -p0 < solaris-types.patch
> +patch -p0 < memory-reporting.patch
\ No newline at end of file

This should be 'patch -p3' and a newline at the end of the file.

Comment 30

[Nicholas Nethercote [inactive]]

> Nick f+'d the patch, but commented that it would be r+

I did? Whoops. Yes, I meant r+.

Comment 31

[Nathan Froyd [:froydnj]]

Attachment: part 1 - indirect libogg memory allocations through variables

Comment 32

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add a memory reporter for vorbis-related allocations

Comment 33

[Nathan Froyd [:froydnj]]

I'm having trouble making this patchset build on Windows, and I don't really understand what the issue is.  (Part 1 has had some changes and will need re-review before it lands for good.  I also need to verify that dragging in gkmedias things at this point doesn't cause startup regressions.)

There are new variables ogg_malloc_function and friends.  These symbols should be exported from gkmedias.dll.  Hence we need to update layout/media/symbols.def.in to include these symbols, and they should be marked DATA.  (One gets rather peculiar errors if they are not marked DATA.)

All fine and good.  Except that linking xul.dll fails with:

Unified_cpp_xpcom_build2.obj : error LNK2001: unresolved external symbol _ogg_malloc_function

and so on for the other new variables.

But I can see that gkmedias.lib exports _ogg_malloc_function and friends.  I confirmed that gkmedias.lib is included on the command line for xul.dll.  I can see exports in gkmedias.dll:

  4038  FC5 00767A98 ogg_malloc_function = _ogg_malloc_function

which I assume does the right thing with leading underscores on symbol names and whatnot.

Why can't the link for xul.dll see the new symbols from gkmedias.dll?  These are the first DATA exports added to symbols.def.in, so perhaps there's some subtlety involved?  Pinging dmajor as a Windows person and glandium if he happens to know what's going on and gets to this before dmajor does.

Comment 34

[(Away)]

I poked around on MSDN and found this. I don't know if it applies here but it may be worth a try.

> Note that when you export a variable from a DLL with a .def file, you do not need to specify
> __declspec(dllexport) on the variable. However, in any file that uses the DLL, you must still use
> __declspec(dllimport) on the declaration of data.

(http://msdn.microsoft.com/en-us/library/hyx1zcd3.aspx)

Comment 35

[Nathan Froyd [:froydnj]]

Attachment: part 1 - indirect libogg memory allocations through variables

OK, this has enough bits changed that a re-review is probably justified.
Changes:

- include <stddef.h> in os_types.h for size_t;
- include __declspec() and symbols.def.in bits;
- include moz.build to make __declspec effective;
- update patch file.

The symbols.def.in change is unnecessary given the __declspec() bits, but it
seems consistent to have it.  The __declspec annotations do trigger warnings
while linking gkmedias.dll, since libogg gets compiled with dllexport and other
files in gkmedias.dll get compiled with dllimport.  But this appears to be
harmless, so I have chosen to ignore it.

Note that the patch file ought to be applied with -p0; the diff --git lines
have full paths, but the ---/+++ lines, which are the ones patch cares
about--use the correct, media/libogg/-relative paths.

Comment 36

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add a memory reporter for vorbis-related allocations

This hasn't really changed.  Talos results appear to be OK with touching
gkmedias.dll bits this early, which makes sense since we don't delayload
gkmedias.dll AFAICS.  Carrying over r+.

Comment 37

[Mike Hommey [:glandium]]

Comment on attachment 8383889 [details] [diff] [review]
part 1 - indirect libogg memory allocations through variables

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

::: media/libogg/include/ogg/os_types.h
@@ +16,5 @@
>   ********************************************************************/
>  #ifndef _OS_TYPES_H
>  #define _OS_TYPES_H
>  
> +#include <stddef.h>

Not sure why you need stddef.h.

@@ +25,5 @@
> +   require the __declspec magic to work correctly. */
> +#if defined(_WIN32)
> +   /* When compiling libogg itself, we need to declare exports.
> +      When providing definitions for clients, we need to declare imports. */
> +#  if defined(OGG_BUILDING)

Where is OGG_BUILDING defined?

@@ +38,5 @@
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +extern OGG_EXPORT_DATA void* (*ogg_malloc_function)(size_t);

s/_function/_hook/ maybe, to make it more like malloc_hook and friends from glibc?

Comment 38

[Ralph Giles (:rillian)]

Comment on attachment 8383889 [details] [diff] [review]
part 1 - indirect libogg memory allocations through variables

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

Hmm. I don't know much about dllimport/dllexport, but glandium asks good questions.

DATA looks like the right thing for variables. Do you need the explicit __declspec(dllimport) in the caller case? Have a look at media/libopus/include/opus_defines.h. It uses dllexport for public functions in the public headers when OPUS_BUILD is defined (in media/libopus/moz.build) to compile the library code, but leaves them unmarked (dllimport default?) otherwise. I note we still have to export the entry points in symbols.def.in, so maybe the .def file overrides?

Does it link if you leave the hooks internal to the library and export a setter function instead?

Thanks for the correction on the patch -p 0, I was wrong there.

Comment 39

[Nathan Froyd [:froydnj]]

(In reply to Ralph Giles (:rillian) from comment #38)
> Does it link if you leave the hooks internal to the library and export a
> setter function instead?

I haven't tried, but I'm pretty sure it would, so long as the setter was exported via symbols.def.in.  The only badness would arise from referencing the new hook variables from outside gkmedias, and we never do that without this patch.  We could use a setter function instead, if you would prefer.  That would make things a little easier.

(In reply to Mike Hommey [:glandium] from comment #37)
> ::: media/libogg/include/ogg/os_types.h
> @@ +16,5 @@
> >   ********************************************************************/
> >  #ifndef _OS_TYPES_H
> >  #define _OS_TYPES_H
> >  
> > +#include <stddef.h>
> 
> Not sure why you need stddef.h.

Something needs to define size_t.  stddef.h seemed as convenient a place to get it from as any.

> @@ +25,5 @@
> > +   require the __declspec magic to work correctly. */
> > +#if defined(_WIN32)
> > +   /* When compiling libogg itself, we need to declare exports.
> > +      When providing definitions for clients, we need to declare imports. */
> > +#  if defined(OGG_BUILDING)
> 
> Where is OGG_BUILDING defined?

That's what I get for porting patches over from my Windows machine manually; it was supposed to be defined in moz.build.  If we go with the setter function, we won't need OGG_BUILDING in any event.

> @@ +38,5 @@
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +extern OGG_EXPORT_DATA void* (*ogg_malloc_function)(size_t);
> 
> s/_function/_hook/ maybe, to make it more like malloc_hook and friends from
> glibc?

I am -0 on this change; "hook" makes me think of something that will be called in addition to the normal function, but maybe that's just Emacs background showing through.  But I don't think it's worth arguing about.  Ralph can make the final call here.

Comment 40

[Ralph Giles (:rillian)]

(In reply to Nathan Froyd (:froydnj) from comment #39)

> I haven't tried, but I'm pretty sure it would, so long as the setter was
> exported via symbols.def.in.  The only badness would arise from referencing
> the new hook variables from outside gkmedias, and we never do that without
> this patch.  We could use a setter function instead, if you would prefer. 
> That would make things a little easier.

My instinct is to go with a setter, assuming you can get that to work without __declspec. We'd get a link error on _ogg_malloc if e.g. we moved libvorbis to a separate library, right?

Argument: to export the function pointers in our build we need the OGG_EXPORT decorators; to upstream them we should apply them to all the public entry points so upstream can drop its .def file. Which would be fine, but sounds like more work. Exporting the setter is a less invasive change.

> Something needs to define size_t.  stddef.h seemed as convenient a place to
> get it from as any.

Ah, for size_t in the function prototypes. Got it.

> > s/_function/_hook/ maybe, to make it more like malloc_hook and friends from
> > glibc?
> 
> I am -0 on this change; "hook" makes me think of something that will be
> called in addition to the normal function, but maybe that's just Emacs
> background showing through.  But I don't think it's worth arguing about. 
> Ralph can make the final call here.

I don't really care. __malloc_hook works like this, so that would be appropriate. If you want me to decide, call them 'ogg_malloc_func' to match upstream convention. Short like hook, definitely a function replacement pointer.

Comment 41

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd (:froydnj) from comment #39)
> That's what I get for porting patches over from my Windows machine manually;
> it was supposed to be defined in moz.build.  If we go with the setter
> function, we won't need OGG_BUILDING in any event.

You'll still need to export that symbol. Yes, you can do that with symbols.def.in, but that file is not upstream. You want this upstreamed, don't you?

Comment 42

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #41)
> (In reply to Nathan Froyd (:froydnj) from comment #39)
> > That's what I get for porting patches over from my Windows machine manually;
> > it was supposed to be defined in moz.build.  If we go with the setter
> > function, we won't need OGG_BUILDING in any event.
> 
> You'll still need to export that symbol. Yes, you can do that with
> symbols.def.in, but that file is not upstream. You want this upstreamed,
> don't you?

I think for the upstream version, we are going to need full-on function wrappers so that using a system libogg works correctly.  And since, AFAICS, the upstream version doesn't bother with getting __declspec correct, I'm not going to worry about it myself.  (If upstream suddenly decides to do so, then of course I will write the patches to respect that.)

Comment 43

[Nathan Froyd [:froydnj]]

Attachment: part 1 - indirect libogg memory allocations through variables

OK, here's an updated patch that:

- calls things ogg_${STDLIB_FUNCTION}_func;
- adds an ogg_set_mem_functions function for setting them;
- exports said function through symbols.def.in.

I think I made everybody happy.  I don't think I need anybody's review except
for Ralph's, since non-ogg-related changes are simply adding things to lists
and whatnot.

Comment 44

[Ralph Giles (:rillian)]

Comment on attachment 8387103 [details] [diff] [review]
part 1 - indirect libogg memory allocations through variables

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

r=me with the update.sh diff out of memory-reporting.patch. You should also update README_MOZILLA to mention ogg_alloc.c, or remove the sentence referring to which files are added.

::: media/libogg/memory-reporting.patch
@@ +100,5 @@
> ++  ogg_calloc_func = calloc_func;
> ++  ogg_realloc_func = realloc_func;
> ++  ogg_free_func = free_func;
> ++}
> +diff --git a/media/libogg/update.sh b/media/libogg/update.sh

update.sh doesn't need to patch itself.

::: media/libogg/src/ogg_alloc.c
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim:set ts=4 sw=4 sts=4 ci et: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Can this file be BSD so upstream/other users can take it? That should be fine per our policy.

Comment 45

[Nathan Froyd [:froydnj]]

(In reply to Ralph Giles (:rillian) from comment #44)
> r=me with the update.sh diff out of memory-reporting.patch. You should also
> update README_MOZILLA to mention ogg_alloc.c, or remove the sentence
> referring to which files are added.

I knew I should have made my filterdiff invocation smarter.

"...those applied by update.sh" doesn't sufficiently cover the creation of ogg_alloc.c?

> ::: media/libogg/src/ogg_alloc.c
> @@ +1,5 @@
> > +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > +/* vim:set ts=4 sw=4 sts=4 ci et: */
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> Can this file be BSD so upstream/other users can take it? That should be
> fine per our policy.

Um.  I have no problem with BSD licensing, but which policy are you referring to?  Do we just apply the same upstream license (presumably by pasting in the Xiph.org header found in other files)?

Comment 46

[Ralph Giles (:rillian)]

(In reply to Nathan Froyd (:froydnj) from comment #45)

> "...those applied by update.sh" doesn't sufficiently cover the creation of
> ogg_alloc.c?

It goes on to say the only things added are moz.build and Makefile.in (which is no longer present!) Best just to remove the sentence.

Of course the patch still doesn't apply cleanly in update because it adds a new ogg_alloc.c which is already present in tree. I don't have a good solution for that other than upstreaming though, so I'm fine with that error. I guess you could leave ogg_alloc.c out of the patch as well.

> Um.  I have no problem with BSD licensing, but which policy are you
> referring to?  Do we just apply the same upstream license (presumably by
> pasting in the Xiph.org header found in other files)?

Yes, that's what I meant.

"When modifying or adding to Third Party Code that has already been checked into a Mozilla Repository, you should use the license pertaining to that code." -- https://www.mozilla.org/MPL/license-policy.html#Licensing_of_Third_Party_Code

Comment 47

[Nathan Froyd [:froydnj]]

(In reply to Ralph Giles (:rillian) from comment #46)
> (In reply to Nathan Froyd (:froydnj) from comment #45)
> 
> > "...those applied by update.sh" doesn't sufficiently cover the creation of
> > ogg_alloc.c?
> 
> It goes on to say the only things added are moz.build and Makefile.in (which
> is no longer present!) Best just to remove the sentence.

Ah!  Thanks for clearing that up.

> Of course the patch still doesn't apply cleanly in update because it adds a
> new ogg_alloc.c which is already present in tree. I don't have a good
> solution for that other than upstreaming though, so I'm fine with that
> error. I guess you could leave ogg_alloc.c out of the patch as well.

I can take on upstreaming this (or something like it) too.  I'll try to figure out where to submit patches.

> > Um.  I have no problem with BSD licensing, but which policy are you
> > referring to?  Do we just apply the same upstream license (presumably by
> > pasting in the Xiph.org header found in other files)?
> 
> Yes, that's what I meant.
> 
> "When modifying or adding to Third Party Code that has already been checked
> into a Mozilla Repository, you should use the license pertaining to that
> code." --
> https://www.mozilla.org/MPL/license-policy.html#Licensing_of_Third_Party_Code

Thanks for the clarification!  Will update the license header.

Comment 48

[Ralph Giles (:rillian)]

For upstreaming patches, you should post to http://lists.xiph.org/mailman/listinfo/ogg-dev. Although xiphmont, the maintainer, also has an account.

I think the problem which needs solving for upstream is how to share the allocator variables with other libraries/clients across dll boundaries. The issue we don't have because they're all linking into gkmedias in Firefox.

I'd start by patching in an OGG_BUILD define to make OGG_EXPORT resolve to __declspec(dllexport) as you had in your first patch, and then switch upstream's build to use that instead of the def file and derived version-info script. Then you could export the variables as well as the setter in a separate patch. I'm not sure how to handle the resulting version skew issues.

Comment 49

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54c0440b476f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f84cf36472d3

Comment 50

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/54c0440b476f
https://hg.mozilla.org/mozilla-central/rev/f84cf36472d3