Closed Bug 908503 Opened 11 years ago Closed 10 years ago

Update demuxer in MP4Reader to be usable for <video>

Categories

(Core :: Audio/Video, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: cpearce, Assigned: ajones)

References

Details

Attachments

(7 files, 22 obsolete files)

2.89 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
173.87 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
705.48 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
4.97 KB, patch
glandium
: review+
Details | Diff | Splinter Review
174.94 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
150.15 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
25.68 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
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.
Summary: Use a custom MP4 demuxer for <video> → Include an MP4 demuxer in Gecko for <video>
Assignee: nobody → cpearce
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.
Thanks Cyril. Since GPAC is licensed with LGPL its license is compatible with Gecko, provided we load it dynamically. I'll look into it.
Depends on: 930829
Depends on: 933579
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.
Summary: Include an MP4 demuxer in Gecko for <video> → Update demuxer in MP4Reader to be usable for <video>
Attached patch WIP for building libstagefright (obsolete) — Splinter Review
To use this patch you need to run:

    $ media/libstagefright/checkout.sh
Assignee: cpearce → ajones
Status: NEW → ASSIGNED
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.
Attached patch Part 3 - Add libstagefright (obsolete) — Splinter Review
To use this patch you need to run:

    $ media/libstagefright/checkout.sh
Attachment #8399211 - Attachment is obsolete: true
Attachment #8400416 - Attachment is obsolete: true
Attachment #8403110 - Attachment is obsolete: true
Attached patch Part 1 - Clean up MP4Reader (obsolete) — Splinter Review
Attachment #8403788 - Flags: feedback?(cpearce)
Attachment #8403111 - Attachment is obsolete: true
Attached patch Part 2 - Add libstagefright (obsolete) — Splinter Review
Attachment #8403789 - Flags: feedback?(cpearce)
Attachment #8403112 - Attachment is obsolete: true
Attached patch Part 3 - Use stagefright demuxer (obsolete) — Splinter Review
Attachment #8403790 - Flags: feedback?(cpearce)
Attachment #8403113 - Attachment is obsolete: true
Attached patch Part 4 - Add #define wonder (obsolete) — Splinter Review
Attachment #8403791 - Flags: feedback?(cpearce)
Attachment #8403114 - Attachment is obsolete: true
These patches should probably be run through clang-format.
Attachment #8403788 - Flags: feedback?(cpearce) → feedback+
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.
Attachment #8403790 - Flags: feedback?(cpearce) → feedback+
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.
Attachment #8403791 - Flags: feedback?(cpearce) → feedback-
Attachment #8403789 - Flags: feedback?(cpearce) → feedback+
Attachment #8403788 - Attachment is obsolete: true
Attachment #8403789 - Attachment is obsolete: true
Attachment #8403790 - Attachment is obsolete: true
Attachment #8403791 - Attachment is obsolete: true
Attachment #8408129 - Attachment is obsolete: true
Attachment #8408129 - Flags: review?(cpearce)
Attachment #8408131 - Attachment is obsolete: true
Attachment #8408131 - Flags: review?(cpearce)
Attachment #8408136 - Attachment is obsolete: true
Attachment #8408136 - Flags: review?(cpearce)
Attachment #8408128 - Flags: review?(cpearce) → review+
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.
Attachment #8408138 - Flags: review?(cpearce) → review-
Attachment #8408139 - Flags: review?(cpearce) → review+
Attachment #8408139 - Attachment description: Part 3 - Remove Chrome demuxer; → Part 4 - Remove Chrome demuxer;
Attachment #8408138 - Attachment is obsolete: true
Attachment #8408139 - Attachment description: Part 4 - Remove Chrome demuxer; → Part 5 - Remove Chrome demuxer;
Attachment #8419159 - Flags: review?(cpearce) → review+
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
Attachment #8419164 - Flags: review?(mh+mozilla) → feedback+
(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 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.
Attachment #8419163 - Flags: review?(cpearce) → review-
Attachment #8419164 - Attachment is obsolete: true
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?
Attachment #8419215 - Flags: review?(mh+mozilla) → review+
Fixed all the review recommendations except for fast seeking which requires further investigation.
Attachment #8419227 - Flags: review?(cpearce)
Attachment #8419163 - Attachment is obsolete: true
(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;
}
Attachment #8419227 - Attachment is obsolete: true
Attachment #8419227 - Flags: review?(cpearce)
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?
Attachment #8419779 - Flags: review?(cpearce) → review-
Attachment #8419779 - Attachment is obsolete: true
(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 on attachment 8419920 [details] [diff] [review]
Part 3 - Change to stagefright demuxer;

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

Ship it.
Attachment #8419920 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/ca32138df109
https://hg.mozilla.org/mozilla-central/rev/05fe24d0499a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8421347 - Flags: review?(cpearce) → review+
(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".
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
(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.
Flags: needinfo?(ajones)
Attachment #8421347 - Attachment is obsolete: true
Attachment #8422882 - Flags: review?(cpearce) → review+
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?
Attachment #8424620 - Attachment is obsolete: true
Attachment #8424620 - Flags: review?(cpearce)
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.
Attachment #8425089 - Flags: review?(cpearce) → review-
The 2 refers to SPS then PPS so I have rewritten it to make it more obvious
Attachment #8426000 - Flags: review?(cpearce)
Attachment #8425089 - Attachment is obsolete: true
Comment on attachment 8426000 [details] [diff] [review]
Part 7 - Add conversion to Annex B

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

Ship it.
Attachment #8426000 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/f75676ac4f7d
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Depends on: 1014613
Depends on: 1014626
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?
Flags: needinfo?(ajones)
sorry, missed bug 1014626
Flags: needinfo?(ajones)
Depends on: 1015487
Blocks: 1015487
No longer depends on: 1015487
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?
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.
(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.
(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.
Flags: sec-review?(ptheriault)
Depends on: 1068055
No longer depends on: 1068055
Depends on: 1153730
Depends on: CVE-2015-4480
Depends on: 1179497
Flags: sec-review?(ptheriault)
You need to log in before you can comment on or make changes to this bug.