960873 - [b2g] mp3 buffers extremely memory inefficient with jemalloc

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

While investigating bug 957346 I noticed some odd memory behavior.  While the about-memory report showed 16MB of decoded-audio for the poppit app, there seemed to be another 15MB of heap-unclassified that went with it.  If I cleared the buffers, this heap-classified went away.

I spent a long time thinking this was due to OMX decoder resources or something, but after a day of investigation that appears not to be the case.

Instead I found we are losing significant amount of memory due to a mismatch between the mp3 frame size (4608 bytes) and the jemalloc arena bins (4096 and 8192 in this case).

After the OMX decoder produces a frame we allocate a buffer of matching size here:

  http://mxr.mozilla.org/mozilla-central/source/content/media/omx/MediaOmxReader.cpp#323

Using code like:

  nsAutoArrayPtr<AudioDataValue> buffer(new AudioDataValue[frame.mSize/2] );

This results in a heap allocation of 4608 bytes.  It appears, however, jemalloc really gives us a buffer of 8192 bytes.  For example, see the comments here:

  http://mxr.mozilla.org/mozilla-central/source/memory/mozjemalloc/jemalloc.c#47

Specifically:

  * Allocation requests are rounded up to the nearest size class, and no record
  * of the original request size is maintained.

I tested this by hardcoding an 8000 byte allocation instead of the 4608.  The app memory usage b2g-info and about-memory remained the same.

Unfortunately the 4608 byte mp3 frame size is just a poor fit with the jemalloc bucket sizes.  We end up wasting 3584 bytes per mp3 frame!  This about 43% waste.  Since apps like games tend to pre-load sound effects to be low-latency this wasted memory can sit around for long periods.

This effect is largely mitigated by Web Audio if the decoded data is stored in a single large buffer and then played with AudioContext.createBufferSource().  Still, HTML5 audio seems widespread so it would be useful to mitigate this memory wastage if we can.

Unfortunately we probably don't want to introduce latency by combining multiple frames into a single heap allocation.  The only two solutions I can think of are:

1) Implement a simple, fixed-size block allocator within the media component for mp3 frames.
2) Add an additional bucket size to jemalloc.

I haven't looked to closely at jemalloc, so I'm not sure how easy it will be to do (2).  The table in the comment does mention some odd sizes like 1020kb, so maybe it has some tunable value for how efficient an allocation has to be to draw from the arena.

I think (1) is clearly distasteful, but would be doable.  We should try to tweak maintain a single allocator to avoid fragmenting our memory if we can, though.

Any other ideas?

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This heavily effects the poppit app and any game using HTML5 audio.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I'm basing the information about 4608 bytes being the largest mp3 frame size on this:

  #define BUFSIZE   8192   // big enough to hold 4608 bytes == biggest mp3 frame

Which is found in the android libstagefright code:

  ./frameworks/base/media/libstagefright/codecs/mp3dec/src/pvmp3_dec_defs.h

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

:njn recommended layering on an arena allocator, such as provided in:

  nsprpub/lib/ds/plarena.h

I'll investigate this tomorrow.

Comment 4

[Nicholas Nethercote [inactive]]

Yeah, this is just unlucky sizing. Adding a new size class to jemalloc sounds hard.

Probably the best bet is to use an arena allocator. If the chunk size is 32KB, you'll fit 7.11 frames per chunk, which is very little wasted space. If these things tend to die together, that's even better, because it's a natural fit for an arena allocator.  See nsprpub/lib/ds/plarena.h.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Looking at plarena.h it appears the trick will be to manage the life-cycle of the allocations in a way that fits with the media code.  Since there is no de-allocation support in plarena.h we'll need to have a good place to release the entire arena.  We'll probably also need to layer on free-list as well to support streamed media.

I think perhaps the best place to do this would be MediaQueue defined here:

  http://mxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderReader.h#321

Each MediaQueue would have its own arena.

I was thinking I could modify the MediaQueue API such that it performs the allocation of T[] arrays like AudioData[] instead of having the client allocate before Push().  It could then perform the correct deallocation for the arena in the Pop() method.  The main downside here is the data would either need to be copied out to be returned from Pop() or require the caller to use Peak() and then Pop().

The arena could be lazily initialized the first time data is Push()'d without an arena present.  The arena could be free'd eagerly when all the buffers have been drained.

Paul, Chris, what do you think of this plan?  Do have any other ideas on a better place to tackle this?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

To clarify, I was planning to PL_InitArenaPool() in the first MediaQueue::Push() call and then PL_ArenaPoolFinish() when the last buffer is drained from MediaQueue::Pop().  The goal here being to truly free the media buffers back to the heap (vs using PL_ARENA_RELEASE() and keeping them in the shared arena pool free list).

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

More thoughts on how to proceed:

1) Create a MediaArenaAllocator class that will contain the plarena.  It will also track when all allocations have been free'd and Finish the plarena to restore memory to the system heap.  A new plarena will then be created if necessary on the next allocation.
2) Add a MediaArenaAllocator mArena to each MediaDecoderReader object.
3) Add a MediaDecoderReader::CreateAudioData() method to replace instances of new AudioData().  This will allow the reader instance to pass its mArena to each AudioData object.
4) Modify the AudioData constructor to take a buffer size parameter instead of a buffer pointer.  A new buffer will then be allocated using the mArena provided in (3) above.  The AudioData will be the sole owner of the buffer memory for its lifetime.  Clients can still access the memory directly to copy the data into the buffer and otherwise manipulate it as done today.
5) The AudioData destructor will use mArena to free the buffer.
6) Modify all instead of new AudioData() to use the new MediaDecoderReader::CreateAudioData() method instead. ***

*** Note: I could only find one place where new AudioData() is used, but not within a MediaDecoderReader object.  This is in the fmp4 decoder.  I'll have to investigate a solution for this.

So instead of code doing this:

  nsAutoArrayPtr<AudioDataValue> buffer = new AudioDataValue[numSamples];
  memcpy(buffer, src, numSamples*2);
  AudioQueue().Push(new AudioData(..., buffer.forget(), ...));

They will instead do:

  nsAutoPtr<AudioData> data = CreateAudioData(..., numSamples, ...);
  memcpy(data->mAudioData, src, numSamples*2);
  AudioQueue().Push(data.forget());

Chris, Paul, let me know if I'm off in the weeds.  :-)

Comment 8

[Andreas Gal :gal]

I think we can fix this with less complexity. To avoid fragmentation, we will submit audio in chunks of 4096 - N bytes. So basically loop over the Push(AudioData) and calculate the right lengths and offsets.

N has to be chosen according to whatever jemalloc's reserve is that it enlarges the allocation by.

This setup will waste very little memory and will also lead to memory being freed faster because we can free the page the moment the last byte is played instead of keeping a large arena alive.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andreas Gal :gal from comment #8)
> This setup will waste very little memory and will also lead to memory being
> freed faster because we can free the page the moment the last byte is played
> instead of keeping a large arena alive.

Well, I was trying to arrange it so that the memory could be free'd after the last sample is played by having a separate arena per media source.  Still, I think you're right that re-framing the data would just be simpler.

Re-framing the 4608 byte buffer would end up using two jemalloc blocks at sizes 4096 and 1024.  This is a savings of 3072 bytes per mp3 frame.

Ideally I think we should unify this logic somewhere so all the various codec providers could use it.  I think this could be done with a new method on AudioData.  I'll take a look on Tuesday.

Are there any assumptions elsewhere in the media code that would prevent re-framing?  For example, do we expect audio and video to remain in sync on a buffer-by-buffer basis?  Should we be concerned about rounding errors in re-calculating the duration of the new frames, etc?  Sorry if these are stupid questions, but I'm still trying to wrap my head around all the different code paths and configurations in media.

Thanks!

Comment 10

[Andreas Gal :gal]

roc can answer your questions I am sure.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Actually, it occurred to me we could make a new C++ type that appears to be a flat 4608 (or whatever length) array, but internally keeps multiple jemalloc heap buffers.  We could call it CompactHeapArray<T> or something.

This would make the fix largely transparent to the media code.  It could also be re-used for other allocation slop issues that might arise.

Comment 12

[Nicholas Nethercote [inactive]]

> Actually, it occurred to me we could make a new C++ type that appears to be
> a flat 4608 (or whatever length) array, but internally keeps multiple
> jemalloc heap buffers.  We could call it CompactHeapArray<T> or something.
> 
> This would make the fix largely transparent to the media code.  It could
> also be re-used for other allocation slop issues that might arise.

I recommend not generalizing this type. It's rare that slop is a big issue for fixed-size allocations. And generalizing it would make it much more complex -- you'll have to build in knowledge of jemalloc's size classes, and then have some kind of algorithm for determining how to break up arbitrary-sized allocations.

Comment 13

[Andreas Gal :gal]

+1 to comment 12. A quick fix here will do.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Sounds good. I will start with a minimal solution just for the OMX decoder. We can generalize to the other decoders later if necessary. Thanks for the guidance!

Comment 15

[Andreas Gal :gal]

Attachment: Draft patch, completely uncompiled and untested.

This is how chunking could work. This should be easily usable from other readers as well. That having said I wonder why we are pulling 16mb of data out of the decoder to begin with. Shouldn't we limit how much data we decode by slowing down the decoder once we have a good amount of data buffered? (lets say 512kb). That should make the impact of padding also much less dramatic. roc?

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andreas Gal :gal from comment #15)
> That having said I wonder why we are pulling 16mb of data
> out of the decoder to begin with. Shouldn't we limit how much data we decode
> by slowing down the decoder once we have a good amount of data buffered?
> (lets say 512kb). That should make the impact of padding also much less
> dramatic. roc?

Just to clarify, this is not 16MB for a single media file.  The app is using duplicate <audio> elements to get multi-channel audio:

  http://phoboslab.org/log/2011/03/multiple-channels-for-html5-audio

In particular this is done in the impactjs game library with a default of 4 duplicate <audio> elements for each sound effect.  The sound effects for the game are only 4MB spread out over 33 mp3 file, but they are each loaded 4 times by the app.

I already have a tech evangelism bug open to work with the game developer to tune/fix this.  See bug 959603.

Also, I've been in contact with the author of impactjs, Dominic Szablewski.  The git version of the library already has support for Web Audio which avoids the duplicate load problem (and this bug).  He was kind enough to give me a copy and I verified it worked well on fxos.  The Web Audio implementation dropped poppit memory usage from 70MB to 40MB.

Dominic plans to release the Web Audio version of impactjs in the next few weeks.  He's also planning to write about the audio memory improvements as part of the release.  We should probably do our own blog to educate the game community about the benefits of web audio over HTML5 for multi-channel sound effects.

Comment 17

[Andreas Gal :gal]

Cool. If you can polish the attached patch and land it we should be good here then. Thanks!

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andreas Gal :gal from comment #17)
> Cool. If you can polish the attached patch and land it we should be good
> here then. Thanks!

Will do.  With the holiday weekend I probably won't get to it until Tuesday.  Thanks!

Comment 19

[Nicholas Nethercote [inactive]]

> While
> the about-memory report showed 16MB of decoded-audio for the poppit app,
> there seemed to be another 15MB of heap-unclassified that went with it.

Most memory reporters use a MallocSizeOf function to measure heap blocks, and those functions include slop bytes caused by jemalloc in their measurement. So I was wondering why this was happening. Turns out the audio reporter computes its memory consumption analytically, which is sub-optimal for exactly this reason. I filed bug 961441 to convert it to use a MallocSizeOf function.

Comment 20

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

Splitting audio samples up into smaller buffers seems like a reasonable solution.

To reduce sloop further, we could make AudioData data blocks be exactly the size required by jemalloc, and just buffer the remaining samples until the next time we push audio into the audio queue and we have enough data to fill a complete block. We'd need to ensure we enqueue the remaining samples in a new block once we reach end of stream.

(In reply to Ben Kelly [:bkelly] from comment #9)
> Are there any assumptions elsewhere in the media code that would prevent
> re-framing?

No. Provided the AudioData blocks are timestamped correctly, the media code should handle AudioData blocks being split into multiple AudioData buffers just fine.

Comment 21

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

Comment on attachment 8362152 [details] [diff] [review]
Draft patch, completely uncompiled and untested.

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

::: content/media/MediaDecoderReader.h
@@ +629,5 @@
> +          size_t chunk_size = malloc_good_size(AudioDataSize(chunk_frames));
> +          // Adjust duration and frames so we allocate the largest number of
> +          // frames into this chunk without going over the chunk_size.
> +          frames = chunk_size / sizeof(AudioDataValue);
> +          duration = duration * frames / aFrames;

We should pass the sample rate into this function, and use FramesToUsecs here instead.

In fact, we should just pass the sample rate into the constructor and calculate the duration in the constructor, rather than relying on the caller to do it.

@@ +644,5 @@
> +                                     aChannels));
> +
> +      // Remove the frames we just pushed into the queue and loop if there is
> +      // more to be done.
> +      aOffset += AudioDataSize(frames);

aOffset is the offset in the uncompressed stream, it's OK if it's approximate, so I'd not bother doing this.

Comment 22

[Nicholas Nethercote [inactive]]

Comment on attachment 8362152 [details] [diff] [review]
Draft patch, completely uncompiled and untested.

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

::: content/media/MediaDecoderReader.h
@@ +622,5 @@
> +        size_t chunk_frames = frames;
> +        do {
> +          chunk_frames /= 2;
> +        } while (chunk_frames > 0 &&
> +                 malloc_good_size(AudioDataSize(chunk_frames)) > size);

It's worth double-checking, but I don't think malloc_good_size() is available on all platforms. Bug 814498 is open for adding moz_malloc_good_size(), but hasn't been resolved. It might not even be able to be implemented on all platforms, in which case you'll need fallback code (e.g. don't bother trying to avoid slop).

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: audio-slop.patch v1

Here's a first cut at a working patch.  Many thanks to Andreas for the starting point.

Just asking for feedback at this point.  I need to split the patch up a bit and I'd like to see if can modify the other codecs to use PushAudioData() as well.

Note: I introduced a copy functor in order to support decoders like webm that appear to use something other than a native, flat buffer.

Comment 24

[Nicholas Nethercote [inactive]]

Comment on attachment 8364660 [details] [diff] [review]
audio-slop.patch v1

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

Seems ok, though I didn't entirely understand it. I suspect cpearce will be able to give a more thorough review.

::: content/media/MediaDecoderReader.h
@@ +631,5 @@
> +public:
> +  /**
> +   * Push audio data into the queue.  The queue is responsible for allocating
> +   * AudioDataValue[] buffers.  The caller must provide a functor to copy
> +   * the data into the buffers.  The function must provide the following

s/function/functor/ ?

@@ +632,5 @@
> +  /**
> +   * Push audio data into the queue.  The queue is responsible for allocating
> +   * AudioDataValue[] buffers.  The caller must provide a functor to copy
> +   * the data into the buffers.  The function must provide the following
> +   * signature:

Nit: why mix C and C++ comments? I'd make this one a C++ comment, and save the wasted line at the top and bottom.

@@ +665,5 @@
> +      uint32_t samples = chunkSize / sizeof(AudioDataValue);
> +      nsAutoArrayPtr<AudioDataValue> buffer(new AudioDataValue[samples]);
> +
> +      // Copy audio data to buffer using caller-provided functor.
> +      uint32_t framesCopied = aCopyFunc(buffer, samples);

Is it possible to copy part of a frame? Would that leave things in a problematic state? It seems like it would. If it's not possible, can that be documented?

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nicholas Nethercote [:njn] from comment #24)
> Seems ok, though I didn't entirely understand it. I suspect cpearce will be
> able to give a more thorough review.

Thanks!  If you are ok with my use of malloc_good_size() and MOZ_MEMORY, then I will direct all future review requests to Chris.

> @@ +632,5 @@
> > +  /**
> > +   * Push audio data into the queue.  The queue is responsible for allocating
> > +   * AudioDataValue[] buffers.  The caller must provide a functor to copy
> > +   * the data into the buffers.  The function must provide the following
> > +   * signature:
> 
> Nit: why mix C and C++ comments? I'd make this one a C++ comment, and save
> the wasted line at the top and bottom.

Ah, I just added this with doxygen style comments out of habit.  I'll convert it to match mozilla style.

> @@ +665,5 @@
> > +      uint32_t samples = chunkSize / sizeof(AudioDataValue);
> > +      nsAutoArrayPtr<AudioDataValue> buffer(new AudioDataValue[samples]);
> > +
> > +      // Copy audio data to buffer using caller-provided functor.
> > +      uint32_t framesCopied = aCopyFunc(buffer, samples);
> 
> Is it possible to copy part of a frame? Would that leave things in a
> problematic state? It seems like it would. If it's not possible, can that be
> documented?

The onus to copy full frames is on the client code providing the functor.  This is due to the fact that different codecs may structure their source frame data differently, so only they know how to copy from it.  I'll add a comment indicating that partial frames will be ignored and must not be copied.

Comment 26

[Nicholas Nethercote [inactive]]

> If you are ok with my use of malloc_good_size() and MOZ_MEMORY,
> then I will direct all future review requests to Chris.

Sounds fine.

> I'll add a
> comment indicating that partial frames will be ignored and must not be
> copied.

Lovely!

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 1: Implement MediaDecoderReader::PushCompactAudioData()

Part 1 patch that implements just the helper method.  Some changes from the previous patch:

1) I renamed it to PushCompactAudioData() to describe why someone would want to use it.
2) I broke out the chunk size code into a separate routine to reduce template bloat.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 2: Make OMX use MediaDecoderReader::PushCompactAudioData()

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 3: Make ogg/vorbis use MediaDecoderReader::PushCompactAudioData()

Two follow-up patches show how the routine is used.  If these look reasonable I can update the other codecs.  In particular, we use opus in b2g which I think is in the webm codec.

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 1: Implement MediaDecoderReader::PushCompactAudioData()

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8365357 [details] [diff] [review]
Part 3: Make ogg/vorbis use MediaDecoderReader::PushCompactAudioData()

I'm going to obsolete the ogg/vorbis patch.  I don't think there is evidence of enough of a benefit here to offset the code complexity increase.  My testing of the ogg/vorbis sounds used in b2g keyboard suggest that this code already does a good job at compacting the buffers.  Its possible other vorbis files might be worse, but so far I have not seen it.

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 3: Reduce AppleMP3Reader MAX_AUDIO_FRAMES to minimize alloc slop.

The AppleMP3Reader class was also very inefficient.  It was allocating 98304 bytes for every 46080 bytes of decoded audio data.

This apple API is a bit different, however, in that we can specify how many frames to copy into a buffer we supply.  The size of this buffer is controlled by the AUDIO_MAX_FRAMES constant.  If we pick this to be something that divides evenly into 4608 and also match an allocation buffer size, then we end up with minimal slop.

This patch reduces AUDIO_MAX_FRAMES to 512 to match these conditions.  With this change we end up only allocating 49152 bytes for the same 46080 bytes of decoded audio.  This effectively cuts our memory footprint in half.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 1: Reframe OMX audio data to reduce alloc slop.

After realizing that 512 frame buffers work very well for MP3 data in comment 32, it occurred to me its probably much simpler to just do the same thing for OMX.

While the MediaDecoderReader::PushCompactAudioData() approach could be used by multiple decoders, its probably not worth the added code complexity.  Each decoder needs to be examined individually and will likely need a slightly different solution.

Also, we have no evidence that the other decoders have this problem.  The issue is severe here due to the 4608 constant defined in the mp3 format.  It seems prudent simply to focus on solving this known problem for mp3 and leave the other decoders for now.

Sorry for the churn on this bug, but I'm going to obsolete the previous patches in favor of this simple patch.

In both cases about-memory shows a 12MB improvement in decoded-audio for the poppit game.

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

https://tbpl.mozilla.org/?tree=Try&rev=71069d5d8a7e

Comment 35

[Nicholas Nethercote [inactive]]

Comment on attachment 8366093 [details] [diff] [review]
Part 1: Reframe OMX audio data to reduce alloc slop.

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

::: content/media/omx/MediaOmxReader.cpp
@@ +334,2 @@
>  
> +  // OMX will often hand us an awkward number of frames, such as 4608.  This

4608 is the number of bytes, not the number of frames, right?

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nicholas Nethercote [:njn] from comment #35)
> 4608 is the number of bytes, not the number of frames, right?

Hmm, yes.  According to wikipedia there are 1152 samples per block in layer II and III mpeg audio.  At 16-bit PCM with two channels this results in 4608 bytes.

I'll revisit these constants to make sure I didn't get confused.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

http://en.wikipedia.org/wiki/Elementary_stream#General_layout_of_MPEG-1_audio_elementary_stream

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

From talking with Chris on IRC, its evident we need to address the issue in more codecs.  In addition to omx and apple, we also have directshow, gstreamer, and wfm that decode mp3 data.

This suggests that we should return to the centralized PushCompressedAudioData() method with a copy functor.  To try to simplify things we will share common copy functors where possible.  At first glance it appears that omx, gstream, and wfm could all use the same functor.

I'll post new patches tomorrow.  Sorry again for the churn.

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 1: Refactor MediaDecoderReader.h into separate headers.

Based on an IRC conversation with Chris, we'd like to put the audio compacting code and copy functors into a separate source file instead of lumping everything into MediaDecoderReader.h.

This patch is the first step towards doing that.  It moves some of the types into separate headers so we can reference them without creating circular dependencies.

Basically:

  MediaData, AudioData, VideoData move to MediaData.h / MediaData.cpp
  MediaInfo, AudioInfo, VideoInfo move to MediaInfo.h / MediaInfo.cpp
  MediaQueue moves to MediaQueue.h

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 1: Refactor MediaDecoderReader.h into separate headers.

Once more with the new files actually included in the patch.

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 2: Create AudioCompactor class to minimize allocation slop.

This patch adds an AudioCompactor class that wraps a MediaQueue<AudioData> instance.  Pushing to the AudioCompactor will chop up the given buffer into malloc friendly sizes and push the resulting buffers into the queue.

The client must provide a copy functor, but a basic one is provided in AudioCompactor.  This common copy functor can be used when the source data is already in the native byte format; i.e. when the codec can get away with just using memcpy() now.

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 3: Make OMX use AudioCompactor.

This patch modifies the omx codec to use the new AudioCompactor with the provided native copy functor.

I'll work on pushing this out to the other mp3 codecs tomorrow.

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug960873_part4_gstreamer.patch

Enable AudioCompactor in gstreamer.  Tested locally on ubuntu 12.04.

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 1: Refactor MediaDecoderReader.h into separate headers. (v2)

Rebased for ImageFormat changes.

Comment 45

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 2: Create AudioCompactor class to minimize allocation slop. (v2)

Re-based for header re-arrangement in Part 1.

Comment 46

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 2: Create AudioCompactor class to minimize allocation slop. (v3)

I had to update the AudioCompactor patch to deal with the annoying problem of winbase.h #defining GetCurrentTime to GetTickCount.  The new AudioCompactor.cpp apparently causes the headers to get processed in a slightly different order permitting the bogus macro to leak through to MediaDecoderStateMachine.cpp.

To avoid this I added the #undef to MediaDecoderStateMachine.cpp.  I also added it to MediaDecoderStateMachine.h, since it seems like it should probably be there anyway given how its done in MediaDecoder.h.

Unfortunately, with unified builds, though, putting it in the headers is not enough.

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 4: Make gstreamer use AudioCompactor. (v2)

Fix the gstreamer patch commit message.

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 5: Make DirectShow use AudioCompactor.

Modify DirectShow codec to use AudioCompactor.  This uses a custom copy functor as the buffer is not in a flat, native format.

Tested on windows 8.1 with an mp3, although I'm not sure if I hit the single byte or two byte sample case.  Apparently I don't know how to get printf_stderr() to show on windows.  I added an exit() call to the copy functor, though, to verify I was actually hitting the new code.

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

Try run for patches to this point:

  https://tbpl.mozilla.org/?tree=Try&rev=9cacaa97296f

I want to write a gtest for the AudioCompactor, do a little more clean up, and then I will be ready for review.

Comment 50

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 2: Create AudioCompactor class to minimize allocation slop. (v4)

Add debug assertions and general code cleanup.

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 3: AudioCompactor gtest unit tests. (v1)

gtest unit tests for the AudioCompactor.

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 4: Make OMX use AudioCompactor. (v2)

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 5: Make gstreamer use AudioCompactor. (v3)

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 5: Make DirectShow use AudioCompactor. (v2)

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 7: Reduce apple mp3 MAX_AUDIO_FRAMES to minimize alloc slop.

This patch does not use AudioCompactor, but instead just tweaks the constant in AppleMP3Reader.cpp.  This is ultimately simpler for this class and most memory efficient.  In order to use AudioCompactor we would need to add an additional copy to the process.

Comment 56

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 6: Make DirectShow use AudioCompactor. (v3)

Fix commit log message.

Comment 57

[Ben Kelly [:bkelly, not reviewing]]

Combined try build:

  https://tbpl.mozilla.org/?tree=Try&rev=3ce8177f9545

Comment 58

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

Comment on attachment 8368833 [details] [diff] [review]
Part 1: Refactor MediaDecoderReader.h into separate headers. (v2)

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

::: content/media/MediaInfo.cpp
@@ +15,5 @@
> +using layers::PlanarYCbCrImage;
> +using layers::PlanarYCbCrData;
> +
> +bool
> +VideoInfo::ValidateVideoRegion(const nsIntSize& aFrame,

It seems like we could just put this in VideoUtils.cpp instead, and rename it IsValidVideoRegion().

Then we won't need another file just for a single function.

Comment 59

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

Comment on attachment 8368833 [details] [diff] [review]
Part 1: Refactor MediaDecoderReader.h into separate headers. (v2)

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

Also, according to the style guide new files should have a different mode line than what you're using (and what's used in the vast majority of the code in content/media/ FWIW).

The mode line should be:
/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
/* vim: set ts=8 sts=2 et sw=2 tw=80: */
/* 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/. */

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Mode_Line

But you're using "tab-width: 2". Please fix that in this patch and other patches before landing.

I only discovered when I requested review last week on some code! ;)

Comment 60

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

Comment on attachment 8369251 [details] [diff] [review]
Part 2: Create AudioCompactor class to minimize allocation slop. (v4)

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

Looks good!

::: content/media/AudioCompactor.cpp
@@ +14,5 @@
> +static size_t
> +MallocGoodSize(size_t aSize)
> +{
> +# if defined(MOZ_MEMORY)
> +    return malloc_good_size(aSize);

Two space indentation level please.

::: content/media/AudioCompactor.h
@@ +13,5 @@
> +namespace mozilla {
> +
> +class AudioCompactor {
> +public:
> +  AudioCompactor(MediaQueue<AudioData> &aQueue) :

The '&' and '*' in pointers and references should be left hugging, not right hugging, in Gecko code.

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Operators

So make this:
 AudioCompactor(MediaQueue<AudioData>& aQueue)

There are other instances which need to change too.

@@ +70,5 @@
> +
> +    return true;
> +  }
> +
> +  // Copy functor suitable for source buffers already in native byte format.

// Copy functor suitable for copying audio samples already in the AudioDataValue format/layout expected by AudioStream on this platform. 

(We don't want to confuse audio float/PCM/planar issues with endianess issues here)

Comment 61

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

Need also to do content/media/plugins (android/fennec MP3).

Comment 62

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

Comment on attachment 8369256 [details] [diff] [review]
Part 7: Reduce apple mp3 MAX_AUDIO_FRAMES to minimize alloc slop.

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

::: content/media/apple/AppleMP3Reader.cpp
@@ +14,5 @@
>  // Maximum number of audio frames we will accept from the audio decoder in one
> +// go.  Carefully select this to work well with both the mp3 1152 max frames
> +// per block and power-of-2 allocation sizes.  Since we must pre-allocate the
> +// buffer, choosing a value that divides exactly into 1152 is most memory
> +// efficient.

Add a note saying that in doing so we avoid the need to use the AudioDataCompactor.

Comment 63

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

Comment on attachment 8369257 [details] [diff] [review]
Part 6: Make DirectShow use AudioCompactor. (v3)

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

::: content/media/directshow/DirectShowReader.cpp
@@ +251,5 @@
> +{
> +public:
> +  DirectShowCopy(uint8_t *aSource, uint32_t aBytesPerSample,
> +                 uint32_t aSamples, uint32_t aChannels) :
> +    mSource(aSource),

Going forward, constructor initalizers are supposed to be indented like so:

  MyClass()
    : mVar(0)
    , mVar2(0)

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Whitespace

There are probably other instances of this, please update them before landing.

@@ +265,5 @@
> +    size_t maxSamples = std::min(aSamples, mSamples - mNextSample);
> +    uint32_t frames = maxSamples / mChannels;
> +    size_t byteOffset = mNextSample * mBytesPerSample;
> +    for (uint32_t i = 0; i < maxSamples; ++i) {
> +      if (mBytesPerSample == 1) {

Do the if (mBytes...) outside the loop, so we're not re-doing the comparison unnecessarily.

Comment 64

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

(In reply to Chris Pearce (:cpearce) from comment #63)
> Comment on attachment 8369257 [details] [diff] [review]
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Coding_Style#Whitespace
> 
> There are probably other instances of this, please update them before
> landing.

Note: I only expect you to change the instances in your patches, I'm not asking you to change all of content/media/ to conform to the new guidelines!

Comment 65

[Ben Kelly [:bkelly, not reviewing]]

Thanks for the review!  I'll get these items fixed up in the morning.

Also, it appears the new gtest is failing on tbpl.  I'll investigate and fix that as well.

For the android/fennec codec I'll need help testing.  I don't have an android device at the moment.

Comment 66

[Ben Kelly [:bkelly, not reviewing]]

The gtest failed on tbpl because some of the build targets have trace-malloc enabled instead of jemalloc.  This means there is no |malloc_good_size()| provided and we fallback on not chunking at all.  This works fine, but breaks the assumptions in the gtest assertions.

I'll relax the assertions to check acceptable slop amounts instead of exact numbers of bytes.

I also realized that I forgot to put a test in for NativeCopy.  I'll add that as well.  I'll probably ask for re-review due to the number of changes.

Comment 67

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 1: Refactor MediaDecoderReader.h into separate headers. (v3)

Updated per review comments.  Carrying forward r=cpearce.

I intend to land Part 1 patch immediately once the try build turns green since this kind of patch can bit rot quickly.

  https://tbpl.mozilla.org/?tree=Try&rev=4792ac1a460f

Comment 68

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 1: Refactor MediaDecoderReader.h into separate headers. (v4)

Missed a ValidateVideoRegion() call nested an extra directory level deep.  Updated to fix that and new try:

  https://tbpl.mozilla.org/?tree=Try&rev=637795cd0ebf

Comment 69

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 1: Refactor MediaDecoderReader.h into separate headers. (v5)

Another build fix for moving the function to VideoUtils.  Waiting for tbpl to re-open for the next try run.

Comment 70

[Ben Kelly [:bkelly, not reviewing]]

https://tbpl.mozilla.org/?tree=Try&rev=584afdc09a56

Comment 71

[Ben Kelly [:bkelly, not reviewing]]

Try looked relatively good, so landed part 1 in mozilla-inbound:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/c66ebc941479

Please leave open until I can land the remaining patches.

Comment 72

[Ben Kelly [:bkelly, not reviewing]]

This is needed for bug 965369 which is a blocker.  Nom for 1.3

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c66ebc941479

Comment 74

[Dylan Oliver [:doliver]]

triage: 1.3+ for blocking bug 965369

Comment 75

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8369586 [details] [diff] [review]
Part 1: Refactor MediaDecoderReader.h into separate headers. (v5)

This was previously r=cpearce.  Forgot to re-add the flag when I updated the patch.

Comment 76

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 2: Create AudioCompactor class to minimize allocation slop. (v5)

Updated for review issues.  Carry forward r=cpearce.

Comment 77

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 3: AudioCompactor gtest unit tests. (v2)

Update tests to check for slop threshold instead of exact byte allocations.  This is more flexible and should let us pass tests even when trace_malloc is enabled and preventing us from using malloc_good_size().

I also added a test for the NativeCopy functor.  This was good since it caught an off-by-one error in one of the assertions.

Asking for re-review since its a non-trivially amount of changes to the file.

Comment 78

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 3: AudioCompactor gtest unit tests. (v3)

Update the modeline as previously requested.

Comment 79

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 4: Make OMX use AudioCompactor. (v3)

Update commit log for r=cpearce.

Comment 80

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 5: Make gstreamer use AudioCompactor. (v4)

Update commit log for r=cpearce.

Comment 81

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 6: Make DirectShow use AudioCompactor. (v4)

Fix style issues.  Move conditional outside of tight copy loop.  Update commit log for r=cpearce.

Comment 82

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 7: Reduce apple mp3 MAX_AUDIO_FRAMES to minimize alloc slop. (v2)

Update comment to indicate AudioCompactor should not be used with apple MP3 API.  Update commit log for r=cpearce.

Comment 83

[Ben Kelly [:bkelly, not reviewing]]

One last try build for Part 2 through Part 7:

  https://tbpl.mozilla.org/?tree=Try&rev=b20e55551c72

Once this is green I will land these patches.  I will then work on the android patch.  Again, I will need help testing that patch as I do not have a device.

Comment 84

[Kai-Chih Hu [:khu]]

Can we get ETA to fix this bug? Thank you.

Comment 85

[Ben Kelly [:bkelly, not reviewing]]

Try was green.  Pushed to b2g-inbound since mozilla-inbound was closed for bustage:

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/612db692256b
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/6e3966e0eb40
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/29c36a7acfb0
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/36c8b10b0a31
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/6c9e404aeb3f
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/33d8989841ac

Please leave open as we need one last fix for android.

Comment 86

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/612db692256b
https://hg.mozilla.org/mozilla-central/rev/6e3966e0eb40
https://hg.mozilla.org/mozilla-central/rev/29c36a7acfb0
https://hg.mozilla.org/mozilla-central/rev/36c8b10b0a31
https://hg.mozilla.org/mozilla-central/rev/6c9e404aeb3f
https://hg.mozilla.org/mozilla-central/rev/33d8989841ac

Comment 87

[Ben Kelly [:bkelly, not reviewing]]

Now that the parts relevant to b2g have landed in mozilla-central I'm marking 1.4 fixed.

Ryan, would it be possible to uplift Parts 1 to 4 to b2g28_v1.3 and b2g28_v1.3t now?  The bug is still open because I need to write an android patch, but that shouldn't effect b2g.

Sorry for the ni, but I didn't think your normal scripts would pick this up with the bug still open.

Comment 88

[Ben Kelly [:bkelly, not reviewing]]

It appears this might not stick.  I used size_t for the functor aSamples argument which should probably be a uint32_t instead.  This causes a std::min type mismatch which breaks the compiler on win64.

I think the most correct thing to do would be to backout Part 2 to Part 7 and reland with the correct type.

Clearing ni? until I can get this sorted.

Comment 89

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 2: Create AudioCompactor class to minimize allocation slop. (v6)

Convert aSamples argument to copy function to be uint32_t instead of size_t.

Comment 90

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 6: Make DirectShow use AudioCompactor. (v5)

Convert copy functor aSamples parameter to be uint32_t instead of size_t.  This should fix the win64 compile failure about mixing types in std::min().

Comment 91

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Revert bug 960873 Part 2 (612db692256b) to Part 7 (33d8989841ac)

Patch to revert part 2 through part 7 from mozilla-central.

Try build combining this revert, plus fixed patches:

  https://tbpl.mozilla.org/?tree=Try&rev=0e5d9a07c5bc

Comment 92

[Phil Ringnalda (:philor)]

It was making me nervous having it in, so I backed out 2-7 in https://hg.mozilla.org/mozilla-central/rev/d2d0899bb3bd

Comment 93

[Ben Kelly [:bkelly, not reviewing]]

The try run passed the win64 build successfully.  I think I need to wait for the backout in comment 92 to get merged to b2g-inbound or mozilla-inbound before I land the patches again.

Comment 94

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 8: Use AudioCompactor in android PluginMediaReader.cpp. (v1)

Use AudioCompactor in android PluginMediaReader.  This should look familiar as its essentially the exact same code as the omx codec.

Try run:

  https://tbpl.mozilla.org/?tree=Try&rev=afe057c9c807

I don't have a local build environment or device to test on.

Comment 95

[Ben Kelly [:bkelly, not reviewing]]

Landed Part 2 through Part 7 again in mozilla-inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/41ddd2fd2031
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/703bf8e7c111
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/610a841538e8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f76c2e37823d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/66b4798ac58f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/daa1d43152d7

Comment 96

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/41ddd2fd2031
https://hg.mozilla.org/mozilla-central/rev/703bf8e7c111
https://hg.mozilla.org/mozilla-central/rev/610a841538e8
https://hg.mozilla.org/mozilla-central/rev/f76c2e37823d
https://hg.mozilla.org/mozilla-central/rev/66b4798ac58f
https://hg.mozilla.org/mozilla-central/rev/daa1d43152d7

Comment 97

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Part 8: Use AudioCompactor in android PluginMediaReader.cpp. (v2)

Fix a minor copy/paste naming mistake in the android MediaPluginReader use of AudioCompactor.  Lets not reference Omx in the copy type name as this is not the OMX codec.

Comment 98

[Ben Kelly [:bkelly, not reviewing]]

The refactor in part 1 does not apply to b2g28_v1_3 cleanly.  I need to roll a branch specific patch.

Comment 99

[Ben Kelly [:bkelly, not reviewing]]

Attachment: b2g28_v1_3 Part 1: Refactor MediaDecoderReader.h into separate headers. (v5)

Here is part 1 rebased to mozilla-b2g28_v1_3.

Comment 100

[Ben Kelly [:bkelly, not reviewing]]

Attachment: b2g28_v1_3 Part 2: Create AudioCompactor class to minimize allocation slop. (v6)

Here is part 2 rebased to mozilla-b2g28_v1_3.

Comment 101

[Ben Kelly [:bkelly, not reviewing]]

Ryan, could you merge parts 1, 2, and 4 on mozilla-b2g28_v1_3?  The bug is still open until the android specific code patch gets reviewed.

Parts 1 and 2 had to be rebased for the 1.3 branch.  See:

  attachment 8371627 [details] [diff] [review]
  attachment 8371630 [details] [diff] [review]

Part 4 applies cleanly:

  https://hg.mozilla.org/mozilla-central/rev/610a841538e8

Comment 102

[Ben Kelly [:bkelly, not reviewing]]

Ryan, can you do a targeted uplift to v1.3 while the bug is still open for an android change?  See comment 101.  Thank you!

Comment 103

[Ryan VanderMeulen [:RyanVM]]

Sure, you can just set the whiteboard like I did above for future bugs :)

Comment 104

[Ryan VanderMeulen [:RyanVM]]

Parts 1, 2, and 4 landed on v1.3.

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/8b9f478e1f50
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/91418373be74
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/7d50226ee796

Comment 105

[Ben Kelly [:bkelly, not reviewing]]

Botched the landing in mozilla-inbound:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/de00c8bfc9b9

Then reverted:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/22d850638009

And finally landed again:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/ef1d09ac296a

Comment 106

[Ben Kelly [:bkelly, not reviewing]]

And just to be explicit, once Part 8 is merged this bug can be marked fixed.  Thanks!

Comment 107

[Ryan VanderMeulen [:RyanVM]]

Backed out from v1.3 for Nexus 4 / JB bustage. I tried pinging you on IRC but couldn't get a response :(
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/f62a56198154

https://tbpl.mozilla.org/php/getParsedLog.php?id=34243349&tree=Mozilla-B2g28-v1.3

Comment 108

[Ben Kelly [:bkelly, not reviewing]]

Thanks Ryan.

I think this is due to an additional ImageContainer.h include sneaking into MediaData.h during the rebase.  I didn't realize try server would work on this branch so did not run it through.  I'll get it checked now.

Comment 109

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Fixed b2g28_v1_3 Part 1: Refactor MediaDecoderReader.h into separate headers. (v5)

Updated part 1 refactor patch for b2g28_v1_3.  Try run:

  https://tbpl.mozilla.org/?tree=Try&rev=21ea86fb6376

Comment 110

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ef1d09ac296a

Comment 111

[Ben Kelly [:bkelly, not reviewing]]

Ok, the try run in comment 109 is a bit ugly, but the reds look like failures in the repo sync to me.  There is a green JB emulator debug build, so I believe the build fix is good.

Lets try to land this on mozilla-b2g28_v1_3 again.  Patches are now:

  attachment 8372019 [details] [diff] [review]
  attachment 8371630 [details] [diff] [review]
  https://hg.mozilla.org/mozilla-central/rev/610a841538e8

Comment 112

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/71b516693df4
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/85a39f463b2e
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/e27ed4af5197

Comment 113

[Joe Cheng [:jcheng] (please needinfo)]

status-b2g-v1.3T?, remove [tarako] whiteboard

Comment 114

[Ben Kelly [:bkelly, not reviewing]]

These patches appear to already be in mozilla-b2g_v1.3t:

  http://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/71b516693df4
  http://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/85a39f463b2e
  http://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/e27ed4af5197