908503 - Update demuxer in MP4Reader to be usable for <video>

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

We need to have our own MP4 demuxer in software that we control.

This is because in the WMF backend we have some kind of bug in the WMFByteStream which is causing resource contention and causing us to fail allocating IMFSourceReader objects.

We also need to support fragmented MP4 (bug 886196).

Also having our own demuxer means we have finer control over threading etc.

The basic long term plan is to have a single MP4 backend that runs on all platforms, and have some kind of abstraction layer between us and system decoders. So we can use the same demuxer on all platforms, and have finer control.

I haven't chosen which demuxer to use yet.

Comment 1

[cyril.concolato]

You might want to consider the demultiplexer from the GPAC project. http://gpac.wp.mines-telecom.fr/
The GPAC project has several C-based tools to create or read MP4 files. Support for MP4 features is very good, including for fragmented MP4, DASH, HEVC, WebVTT/TTML in MP4, etc. I'd be happy to have your requirements to see if we can adapt GPAC to your needs.

Comment 2

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

Thanks Cyril. Since GPAC is licensed with LGPL its license is compatible with Gecko, provided we load it dynamically. I'll look into it.

Comment 4

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

Update: the MP4 demuxer used by MP4Reader (chromium's) is used only in Media Source Extensions as it currently only handles fragmented MP4 which conform to the MSE constraints. We'd like to use the MP4Reader for all MP4 playback, so that we have better control over the threading and seeking, and more consistent behaviour across platforms.

One solution would be to use android's demuxer from libstagefright. Another solution would be adding code to our existing demuxer to make it handle the things we want it to.

The advantage of using Android's demuxer is that it also has code to supports the other format's we're interested in on mobile, like MP3, H.263, 3GP, etc.

Ideally if we import a new demuxer, it would be good if we could do it with as few code changes a possible, to make updating it from upstream easier.

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: WIP for building libstagefright

To use this patch you need to run:

    $ media/libstagefright/checkout.sh

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: WIP using stagefright demuxer

Comment 7

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

You can test fMP4 and MSE on this page: http://people.mozilla.org/~cpearce/simple-dash/ make sure you set the prefs media.mediasource.enabled and media.fragmented-mp4.exposed to true.

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 1 - Fix FMP4 priority

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2 - Clean up MP4Reader

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 3 - Add libstagefright

To use this patch you need to run:

    $ media/libstagefright/checkout.sh

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 4 - using stagefright demuxer

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 5 - WIP add #define wonder

Comment 13

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 1 - Clean up MP4Reader

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2 - Add libstagefright

Comment 15

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 3 - Use stagefright demuxer

Comment 16

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 4 - Add #define wonder

Comment 17

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

These patches should probably be run through clang-format.

Comment 18

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

Comment on attachment 8403790 [details] [diff] [review]
Part 3 - Use stagefright demuxer

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

Looks good.

In future we need need to be able to distinguish between an error demuxing and end of stream. I'm adding the ability to report that in bug 979104. It looks like we'll be easily able to add that to the MP4Demuxer's Demux*Sample() functions fortunately.

There's a reason I use (u)intXX_t data types everywhere; our offsets need to be 64bit so we can handle file sizes > 32bit, and in Win32 off_t is only 32bit. So please don't use off_t, use explicitly sized (u)intXX_t in our code.

::: content/media/fmp4/MP4Reader.cpp
@@ +50,5 @@
>    virtual ~MP4Stream() {
>      MOZ_COUNT_DTOR(MP4Stream);
>    }
>  
> +  virtual bool ReadAt(off_t aOffset,

Use int64_t, as off_t is 32bit in Win32 builds, meaning you can't handle files > 4GB.

@@ +70,5 @@
>      *aBytesRead = sum;
>      return true;
>    }
>  
> +  virtual bool GetSize(off_t* aSize) MOZ_OVERRIDE {

Use int64_t. sizeof(off_t) in win32 builds is 4, meaning you can't handle files > 4GB.

@@ +120,5 @@
>    if (mVideo.mTaskQueue) {
>      mVideo.mTaskQueue->Shutdown();
>      mVideo.mTaskQueue = nullptr;
>    }
> +#ifdef MOZ_SF_DEMUXER

You don't need this MOZ_SF_DEMUXER right? Since you're replacing the old demuxer.

@@ +185,2 @@
>    // If we have audio, we *only* allow AAC to be decoded.
> +  if (mInfo.mAudio.mHasAudio && strcmp(audio.mime_type,

Using HasAudio() is more concise than mInfo.mAudio.mHasAudio.

Comment 19

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

Comment on attachment 8403791 [details] [diff] [review]
Part 4 - Add #define wonder

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

I think we should just rip out the old demuxer, assuming MSE content still works, we won't need it.

Comment 20

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 1 - Clean up MP4Reader;

Comment 21

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2 - Change to stagefright demuxer;

Comment 22

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2 - Change to stagefright demuxer;

Comment 23

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2 - Change to stagefright demuxer;

Comment 24

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2 - Change to stagefright demuxer;

Comment 25

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 5 - Remove Chrome demuxer;

Comment 26

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

Comment on attachment 8408138 [details] [diff] [review]
Part 2 - Change to stagefright demuxer;

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

Looks pretty good. Only major problem is in ConvertEsdsToAdts().

Please split this patch into two, one which imports the libstagefright code, and another which changes our code to build it. Please get build peer review on the import patch.

::: content/media/fmp4/MP4Reader.cpp
@@ +69,5 @@
>      *aBytesRead = sum;
>      return true;
>    }
>  
> +  virtual bool GetSize(int64_t* aSize) MOZ_OVERRIDE

Leave this as Length() to be consistent with our other stream abstractions.

@@ +120,5 @@
>    if (mVideo.mTaskQueue) {
>      mVideo.mTaskQueue->Shutdown();
>      mVideo.mTaskQueue = nullptr;
>    }
> +#ifdef MOZ_SF_DEMUXER

You won't need this #ifdef once the chromium demuxer is removed right?

@@ +209,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>  
>    if (HasVideo()) {
> +    mInfo.mVideo.mDisplay = nsIntSize(video.width, video.height);

Please rename VideoDecoderConfig::width and height to make it clear that they are the size at which the frame is displayed, not the size of the frame itself.

@@ +524,5 @@
> +  Flush(kVideo);
> +  Flush(kAudio);
> +  ResetDecode();
> +
> +  mQueuedVideoSample.forget();

mQueuedVideoSample = nullptr;

Otherwise you'll leak, right?

::: content/media/fmp4/MP4Reader.h
@@ +128,5 @@
>      bool mIsFlushing;
>    };
>    DecoderData mAudio;
>    DecoderData mVideo;
> +  // Queue of input extracted by the demuxer, but not yet sent to the

It's not technically correct to call this a queue...

::: content/media/fmp4/wmf/WMFAudioOutputSource.cpp
@@ +67,5 @@
>  
> +WMFAudioOutputSource::WMFAudioOutputSource(
> +  const mp4_demuxer::AudioDecoderConfig& aConfig)
> +  : mAudioChannels(aConfig.channel_count)
> +  , mAudioBytesPerSample(2)

Does the new demuxer support telling us how many bits/bytes per sample? I'm not super keen on assuming it's always 2 bytes per sample.

::: media/libstagefright/binding/Adts.cpp
@@ +40,5 @@
> +  if (newSize > aSample->mMediaBuffer->size() || newSize >= (1 << 13) ||
> +      aChannelCount < 0 || aChannelCount > 15 || aFrequencyIndex < 0)
> +    return false;
> +
> +  // 4 is better, right?

Um, not necessarily... 

4 is AAC LTP:
http://wiki.multimedia.cx/index.php?title=MPEG-4_Audio#Audio_Object_Types

You can get the audio object type from the first five bits of the AudioSpecificConfig:
http://wiki.multimedia.cx/index.php?title=MPEG-4_Audio#Audio_Specific_Config

You should probably be doing that.

::: media/libstagefright/binding/include/mp4_demuxer/DecoderData.h
@@ +56,5 @@
> +  }
> +
> +  const char* mime_type;
> +  int64_t duration;
> +  int32_t width;

call this display_width, so it's clear that this is the size at which is should be displayed.

::: media/libstagefright/binding/mp4_demuxer.cpp
@@ +153,5 @@
> +  }
> +
> +  aSample->Update();
> +  if (!Adts::ConvertEsdsToAdts(mAudioConfig.channel_count,
> +                               mAudioConfig.frequency_index, aSample))

Curly braces around one line if statements please.

Comment 27

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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

Comment 28

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2 - Import libstagefright code from AOSP;

Comment 29

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 3 - Change to stagefright demuxer;

Comment 30

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 4 - Build config changes for libstagefright demuxer;

Comment 31

[Mike Hommey [:glandium]]

Comment on attachment 8419164 [details] [diff] [review]
Part 4 - Build config changes for libstagefright demuxer;

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

How well does that play with the stuff in media/omx-plugin?

::: media/libstagefright/moz.build
@@ +9,5 @@
> +DEFINES['FAKE_LOG_DEVICE'] = True
> +DEFINES['LOG_NDEBUG'] = 0
> +
> +if CONFIG['_MSC_VER']:
> +    DEFINES['OS_PATH_SEPARATOR'] = "'\\\\'"

A cursory look makes me think the code that uses OS_PATH_SEPARATOR is dead code.

@@ +10,5 @@
> +DEFINES['LOG_NDEBUG'] = 0
> +
> +if CONFIG['_MSC_VER']:
> +    DEFINES['OS_PATH_SEPARATOR'] = "'\\\\'"
> +    DEFINES['ssize_t'] = 'int'

This is most likely wrong on Win64.

@@ +20,5 @@
> +    LOCAL_INCLUDES += [ 'ports/win32/include' ]
> +elif CONFIG['OS_TARGET'] == 'Darwin':
> +    DEFINES['OS_PATH_SEPARATOR'] = "'/'"
> +    DEFINES['HAVE_SYS_UIO_H'] = True
> +    DEFINES['off64_t'] = 'int64_t'

off_t would be better. (always 64 bits on BSDs, which Darwin is)
I /think/ that's true on Windows too.

@@ +93,5 @@
> +    ]
> +    LOCAL_INCLUDES += [
> +        'system/core/include',
> +    ]
> + 

trailing whitespace

Comment 32

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Mike Hommey [:glandium] from comment #31)
> > +if CONFIG['_MSC_VER']:
> > +    DEFINES['OS_PATH_SEPARATOR'] = "'\\\\'"
> 
> A cursory look makes me think the code that uses OS_PATH_SEPARATOR is dead
> code.

The approach we want to take is to modify libstagefright as little as possible. This makes it easier to update.

> > +    DEFINES['ssize_t'] = 'int'
> 
> This is most likely wrong on Win64.

http://technet.microsoft.com/en-us/library/bb496995.aspx

Using int32_t means I need #include <stdint.h>

> @@ +20,5 @@
> > +    LOCAL_INCLUDES += [ 'ports/win32/include' ]
> > +elif CONFIG['OS_TARGET'] == 'Darwin':
> > +    DEFINES['OS_PATH_SEPARATOR'] = "'/'"
> > +    DEFINES['HAVE_SYS_UIO_H'] = True
> > +    DEFINES['off64_t'] = 'int64_t'
> 
> off_t would be better. (always 64 bits on BSDs, which Darwin is)
> I /think/ that's true on Windows too.

Ok.

> @@ +93,5 @@
> > +    ]
> > +    LOCAL_INCLUDES += [
> > +        'system/core/include',
> > +    ]
> 
> trailing whitespace

Ok.

Comment 33

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

Comment on attachment 8419163 [details] [diff] [review]
Part 3 - Change to stagefright demuxer;

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

Looks OK, but seeking won't work properly for fastSeek.

::: content/media/fmp4/MP4Reader.cpp
@@ +275,5 @@
> +      if (mQueuedVideoSample)
> +        return mQueuedVideoSample.forget();
> +
> +      return mDemuxer->DemuxVideoSample();
> +    

Trailing whitespace.

::: content/media/fmp4/ffmpeg/FFmpegDataDecoder.h
@@ +28,5 @@
>    virtual nsresult Drain() = 0;
>    virtual nsresult Shutdown() MOZ_OVERRIDE;
>  
>  protected:
> +  nsAutoPtr<uint8_t> mExtraData;

nsAutoArrayPtr? To make it clear this is not pointing to a single uint8_t.

::: media/libstagefright/binding/include/mp4_demuxer/DecoderData.h
@@ +38,5 @@
> +  uint32_t channel_count;
> +  uint32_t bytes_per_sample;
> +  uint32_t samples_per_second;
> +  int8_t frequency_index;
> +  const void* extra_data;

You should add a comment saying what the extra_data is. i.e. for mime_type X this is Y.

Ditto for the video extra data.

@@ +40,5 @@
> +  uint32_t samples_per_second;
> +  int8_t frequency_index;
> +  const void* extra_data;
> +  size_t extra_data_size;
> +  int8_t aac_profile;

It's a bit gross that you have an AAC specific field on a general purpose looking AudioConfig object.

@@ +66,5 @@
> +  int32_t display_height;
> +  const void* extra_data;
> +  size_t extra_data_size;
> +
> +  void Update(android::sp<android::MetaData>& aMetaData, const char* aMimeType);

It seems that the *Config objects should retain a reference to the android::MetaData to ensure that the data they point to isn't destroyed? Or you should copy the data.

@@ +87,5 @@
> +
> +  uint8_t* data;
> +  size_t size;
> +
> +  nsAutoPtr<uint8_t> audio_buffer;

This is unused.

::: media/libstagefright/binding/include/mp4_demuxer/mp4_demuxer.h
@@ +35,5 @@
> +
> +  bool Init();
> +  bool HasValidAudio();
> +  bool HasValidVideo();
> +  int64_t Duration();

Comment describing units duration is reported in.

Or add a "typedef int64_t Microseconds", and use that as the type. Ideally we'd have explicit and type safe classes for various time types...

@@ +37,5 @@
> +  bool HasValidAudio();
> +  bool HasValidVideo();
> +  int64_t Duration();
> +  bool CanSeek();
> +  void Seek(int64_t aTime);

In a comment specify the units that aTime is in. Microseconds presumably. Can this function fail? I'd assume it can. It should report failure.

@@ +41,5 @@
> +  void Seek(int64_t aTime);
> +  MP4Sample* DemuxAudioSample();
> +  MP4Sample* DemuxVideoSample();
> +
> +  AudioDecoderConfig mAudioConfig;

This should not be publicly writable by clients of the API, make these private and accessed by a const& accessor.

::: media/libstagefright/binding/mp4_demuxer.cpp
@@ +131,5 @@
> +
> +void
> +MP4Demuxer::Seek(int64_t aTime)
> +{
> +  mPrivate->mAudioOptions.setSeekTo(

This isn't actually what we want. We want to seek the video to the keyframe/sync point preceding the target, and then seek the audio stream to the time at which the video stream is at. Typically audio can be seeked to every packet (~8ms granularity) but keyframes have a much larger sync point distance (one every 2 seconds for example).

If you don't do this, fastSeek won't be fast.

And remember we may be playing a media with only a video stream or only an audio stream, so you need to handle those cases too.

Comment 34

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 4 - Build config changes for libstagefright demuxer;

Comment 35

[Mike Hommey [:glandium]]

Comment on attachment 8419215 [details] [diff] [review]
Part 4 - Build config changes for libstagefright demuxer;

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

::: media/libstagefright/moz.build
@@ +11,5 @@
> +
> +if CONFIG['_MSC_VER']:
> +    DEFINES['OS_PATH_SEPARATOR'] = "'\\\\'"
> +    DEFINES['ssize_t'] = 'intptr_t'
> +    DEFINES['off64_t'] = 'int64_t'

doesn't off_t work here?

Comment 36

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 3 - Change to stagefright demuxer;

Fixed all the review recommendations except for fast seeking which requires further investigation.

Comment 37

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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

Comment 38

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

(In reply to Mike Hommey [:glandium] from comment #35)
> Comment on attachment 8419215 [details] [diff] [review]
> Part 4 - Build config changes for libstagefright demuxer;
> 
> Review of attachment 8419215 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libstagefright/moz.build
> @@ +11,5 @@
> > +
> > +if CONFIG['_MSC_VER']:
> > +    DEFINES['OS_PATH_SEPARATOR'] = "'\\\\'"
> > +    DEFINES['ssize_t'] = 'intptr_t'
> > +    DEFINES['off64_t'] = 'int64_t'
> 
> doesn't off_t work here?


off_t is 32bits in Win32 builds. We can't use that for offsets, as then the size of files we can handle is limited to 2 or 4 GB.

I tested with this program:

#include <stdio.h>
#include <SYS\TYPES.H>

int main(int argc, char**argv) {
  printf("sizeof(off_t)=%d\n", sizeof(off_t));
  return 0;
}

Comment 39

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 3 - Change to stagefright demuxer;

Comment 40

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

Comment on attachment 8419779 [details] [diff] [review]
Part 3 - Change to stagefright demuxer;

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

Seek is still needs work. :(

::: content/media/fmp4/MP4Reader.cpp
@@ +524,5 @@
> +  Flush(kVideo);
> +  Flush(kAudio);
> +  ResetDecode();
> +
> +  mDemuxer->SeekVideo(aTime);

Is this operation harmless if we only have an audio stream and no video?

I assume that the reason that we don't need to check for seek failure is that this just sets the offset in the stream to read next, so if there's a network error or the MediaResource is closed the failure won't happen until we try to read next?

It seems to me that you should only seek video if HasVideo() is true, and only seek audio if HasAudio() is true, modulo that for the audio case you should seek to the first video frame's timestamp if you have video and PopSample(kVideo) returned non-null.

@@ +526,5 @@
> +  ResetDecode();
> +
> +  mDemuxer->SeekVideo(aTime);
> +  nsAutoPtr<MP4Sample> compressed(PopSample(kVideo));
> +  mQueuedVideoSample = compressed;

Couldn't you just do:

mQueuedVideoSample = PopSample(kVideo);

@@ +528,5 @@
> +  mDemuxer->SeekVideo(aTime);
> +  nsAutoPtr<MP4Sample> compressed(PopSample(kVideo));
> +  mQueuedVideoSample = compressed;
> +
> +  mDemuxer->SeekAudio(compressed->composition_timestamp);

You'll crash if we're seeking a audio only file, or if the file has both audio and video streams but the video stream contains no samples.

You should be null checking the demuxed sample and seeking to aTime if there's no video stream or PopSample(kVideo) returned null.

::: media/libstagefright/binding/include/mp4_demuxer/DecoderData.h
@@ +42,5 @@
> +  uint32_t bytes_per_sample;
> +  uint32_t samples_per_second;
> +  int8_t frequency_index;
> +
> +  // Decoder specific extra data which in the case of H.264 is the AVCC config

This comment should be on the VideoDecoderConfig, and you need an AAC specific one here.

::: media/libstagefright/binding/include/mp4_demuxer/mp4_demuxer.h
@@ +43,5 @@
> +
> +  void SeekAudio(Microseconds aTime);
> +  void SeekVideo(Microseconds aTime);
> +
> +  MP4Sample* DemuxAudioSample();

Add comment that this returns nullptr on end of stream or error. It does right?

Comment 41

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 3 - Change to stagefright demuxer;

Comment 42

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Chris Pearce (:cpearce) from comment #40)
> Seek is still needs work. :(

Perhaps seeking should've been done as a follow up.

> I assume that the reason that we don't need to check for seek failure is
> that this just sets the offset in the stream to read next, so if there's a
> network error or the MediaResource is closed the failure won't happen until
> we try to read next?

Yes. The stream will fail.

> > +  MP4Sample* DemuxAudioSample();
> 
> Add comment that this returns nullptr on end of stream or error. It does
> right?

Yes.

Comment 43

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

Comment on attachment 8419920 [details] [diff] [review]
Part 3 - Change to stagefright demuxer;

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

Ship it.

Comment 44

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca32138df109
https://hg.mozilla.org/integration/mozilla-inbound/rev/05fe24d0499a
https://hg.mozilla.org/integration/mozilla-inbound/rev/191741878690
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f57690f5141
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9b51826b2ff

Comment 45

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Backed out the last 3 patches.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9bf72f2951f
https://hg.mozilla.org/integration/mozilla-inbound/rev/cde1f2a3cfee
https://hg.mozilla.org/integration/mozilla-inbound/rev/299a5a0a5458

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca32138df109
https://hg.mozilla.org/mozilla-central/rev/05fe24d0499a

Comment 47

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 6 - Fix build issues and Hf build

Comment 48

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6f386352d12
https://hg.mozilla.org/integration/mozilla-inbound/rev/30957caad928
https://hg.mozilla.org/integration/mozilla-inbound/rev/add95b3c2e7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/476cd5c9c5a9

Comment 49

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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

Comment 50

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #48)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f6f386352d12
> https://hg.mozilla.org/integration/mozilla-inbound/rev/30957caad928
> https://hg.mozilla.org/integration/mozilla-inbound/rev/add95b3c2e7f
> https://hg.mozilla.org/integration/mozilla-inbound/rev/476cd5c9c5a9

I had to back these out in  https://hg.mozilla.org/integration/mozilla-inbound/rev/1c03dfd4b49b for breaking the Hamachi eng build: https://tbpl.mozilla.org/php/getParsedLog.php?id=39538577&tree=Mozilla-Inbound

Comment 51

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #31)
> Comment on attachment 8419164 [details] [diff] [review]
> Part 4 - Build config changes for libstagefright demuxer;
> 
> Review of attachment 8419164 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How well does that play with the stuff in media/omx-plugin?

I guess the answer is "badly".

Comment 52

[Carsten Book [:Tomcat]]

note this checkin also seems to have broken the ics opt and debug tests with https://tbpl.mozilla.org/php/getParsedLog.php?id=39545387&tree=Mozilla-Inbound

Comment 53

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Mike Hommey [:glandium] from comment #51)
> > How well does that play with the stuff in media/omx-plugin?
> 
> I guess the answer is "badly".

No. That's not an issue. The problem is that the b2g system header files end up earlier in the #include path than the media/libstagefright/ ones. Finding a way around that would fix most if not all of the b2g build issues.

Comment 54

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 6 - Fix build bustage and modify namespace

Comment 55

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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

Comment 56

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 7 - Add conversion to Annex B

Comment 57

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

Comment on attachment 8424620 [details] [diff] [review]
Part 7 - Add conversion to Annex B

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

Did you test that this works on Windows?

::: content/media/fmp4/MP4Reader.cpp
@@ +35,5 @@
>  
>  namespace mozilla {
>  
>  // Uncomment to enable verbose per-sample logging.
> +#define LOG_SAMPLE_DECODE 1

Don't commit this turned on please.

::: media/libstagefright/moz.build
@@ +52,5 @@
>  ]
>  
>  UNIFIED_SOURCES += [
>      'binding/Adts.cpp',
> +    'binding/AnnexB.cpp',

Did you forget to `hg add` AnnexB.cpp? And AnnexB.h?

Comment 58

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 7 - Add conversion to Annex B

Comment 59

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

Comment on attachment 8425089 [details] [diff] [review]
Part 7 - Add conversion to Annex B

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

I bet this doesn't work on Windows.

And you said the AnnexB code wasn't interesting! ;)

::: content/media/fmp4/MP4Reader.cpp
@@ +35,5 @@
>  
>  namespace mozilla {
>  
>  // Uncomment to enable verbose per-sample logging.
> +#define LOG_SAMPLE_DECODE 1

Comment out before landing please.

::: media/libstagefright/binding/AnnexB.cpp
@@ +33,5 @@
> +  Vector<uint8_t> annex_b;
> +
> +  ByteReader reader(aExtraData);
> +  const uint8_t* ptr = reader.Read(5);
> +  if (ptr && ptr[0] == 1)

Braces riding high in "if" statements.

e.g.: 
if () {

@@ +35,5 @@
> +  ByteReader reader(aExtraData);
> +  const uint8_t* ptr = reader.Read(5);
> +  if (ptr && ptr[0] == 1)
> +  {
> +    for (int spsPps = 0; spsPps < 2; spsPps++)

There may not always 2 sps blocks. You should read the sps blocks instead of assuming it's 2.

for example:
http://mxr.mozilla.org/mozilla-central/source/content/media/fmp4/demuxer/box_definitions.cc#333

@@ +51,5 @@
> +        }
> +        annex_b.append(annex_b_delimiter, ArrayLength(annex_b_delimiter));
> +        annex_b.append(ptr, sps_length);
> +      }
> +    }

What about the PPS? We need that on Windows according to the documentation.

Comment 60

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 7 - Add conversion to Annex B

The 2 refers to SPS then PPS so I have rewritten it to make it more obvious

Comment 61

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

Comment on attachment 8426000 [details] [diff] [review]
Part 7 - Add conversion to Annex B

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

Ship it.

Comment 62

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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

Comment 63

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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

Comment 64

[Carsten Book [:Tomcat]]

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

Comment 65

[Bob Clary [:bc] (inactive)]

I am hitting https://hg.mozilla.org/mozilla-central/rev/f75676ac4f7d#l104.40 Unsupported architecture trying to  build on Windows 7 x86_64 using VS 2012. Is there a way to disable this in the mozconfig?

Comment 66

[Bob Clary [:bc] (inactive)]

sorry, missed bug 1014626

Comment 67

[Dave Hylands [:dhylands]]

This bug totally breaks logging on firefox os devices.

I'm also concerned that because it's now linking stagefright into libxul.so how is this going to work on ICS versus JB versus KK devices?

Comment 68

[Dave Hylands [:dhylands]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1015487#c2 for a description of how this winds up totally breaking logging on firefox os devices.

Comment 69

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

(In reply to Dave Hylands [:dhylands] from comment #67)
> This bug totally breaks logging on firefox os devices.
> 
> I'm also concerned that because it's now linking stagefright into libxul.so
> how is this going to work on ICS versus JB versus KK devices?

I thought this internal copy of the demuxer was supposed to be in different namespace, so as to avoid name collisions with the stagefright demuxer in FirefoxOS.

Comment 70

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Chris Pearce (:cpearce) from comment #69)
> (In reply to Dave Hylands [:dhylands] from comment #67)
> > This bug totally breaks logging on firefox os devices.
> > 
> > I'm also concerned that because it's now linking stagefright into libxul.so
> > how is this going to work on ICS versus JB versus KK devices?
> 
> I thought this internal copy of the demuxer was supposed to be in different
> namespace, so as to avoid name collisions with the stagefright demuxer in
> FirefoxOS.

It is in a different namespace due to this exact problem. There has been an issue with liblog because it's a C library.