The default bug view has changed. See this FAQ.

Update demuxer in MP4Reader to be usable for <video>

RESOLVED FIXED in mozilla32

Status

()

Core
Audio/Video
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: cpearce, Assigned: kentuckyfriedtakahe)

Tracking

Trunk
mozilla32
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 22 obsolete attachments)

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
(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Summary: Use a custom MP4 demuxer for <video> → Include an MP4 demuxer in Gecko for <video>
(Reporter)

Updated

4 years ago
Assignee: nobody → cpearce

Comment 1

4 years ago
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.
(Reporter)

Comment 2

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

Updated

4 years ago
Depends on: 930829
(Reporter)

Updated

3 years ago
Depends on: 933579
(Reporter)

Updated

3 years ago
Duplicate of this bug: 988715
(Reporter)

Comment 4

3 years ago
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.
(Reporter)

Updated

3 years ago
Summary: Include an MP4 demuxer in Gecko for <video> → Update demuxer in MP4Reader to be usable for <video>
Created attachment 8399211 [details] [diff] [review]
WIP for building libstagefright

To use this patch you need to run:

    $ media/libstagefright/checkout.sh
Assignee: cpearce → ajones
Status: NEW → ASSIGNED
Created attachment 8400416 [details] [diff] [review]
WIP using stagefright demuxer
(Reporter)

Comment 7

3 years ago
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.
Created attachment 8403110 [details] [diff] [review]
Part 1 - Fix FMP4 priority
Created attachment 8403111 [details] [diff] [review]
Part 2 - Clean up MP4Reader
Created attachment 8403112 [details] [diff] [review]
Part 3 - Add libstagefright

To use this patch you need to run:

    $ media/libstagefright/checkout.sh
Attachment #8399211 - Attachment is obsolete: true
Created attachment 8403113 [details] [diff] [review]
Part 4 - using stagefright demuxer
Attachment #8400416 - Attachment is obsolete: true
Created attachment 8403114 [details] [diff] [review]
Part 5 - WIP add #define wonder
Attachment #8403110 - Attachment is obsolete: true
Created attachment 8403788 [details] [diff] [review]
Part 1 - Clean up MP4Reader
Attachment #8403788 - Flags: feedback?(cpearce)
Attachment #8403111 - Attachment is obsolete: true
Created attachment 8403789 [details] [diff] [review]
Part 2 - Add libstagefright
Attachment #8403789 - Flags: feedback?(cpearce)
Attachment #8403112 - Attachment is obsolete: true
Created attachment 8403790 [details] [diff] [review]
Part 3 - Use stagefright demuxer
Attachment #8403790 - Flags: feedback?(cpearce)
Attachment #8403113 - Attachment is obsolete: true
Created attachment 8403791 [details] [diff] [review]
Part 4 - Add #define wonder
Attachment #8403791 - Flags: feedback?(cpearce)
Attachment #8403114 - Attachment is obsolete: true
These patches should probably be run through clang-format.
(Reporter)

Updated

3 years ago
Attachment #8403788 - Flags: feedback?(cpearce) → feedback+
(Reporter)

Comment 18

3 years ago
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+
(Reporter)

Comment 19

3 years ago
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-
(Reporter)

Updated

3 years ago
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
Created attachment 8408128 [details] [diff] [review]
Part 1 - Clean up MP4Reader;
Attachment #8408128 - Flags: review?(cpearce)
Created attachment 8408129 [details] [diff] [review]
Part 2 - Change to stagefright demuxer;
Attachment #8408129 - Flags: review?(cpearce)
Created attachment 8408131 [details] [diff] [review]
Part 2 - Change to stagefright demuxer;
Attachment #8408131 - Flags: review?(cpearce)
Attachment #8408129 - Attachment is obsolete: true
Attachment #8408129 - Flags: review?(cpearce)
Created attachment 8408136 [details] [diff] [review]
Part 2 - Change to stagefright demuxer;
Attachment #8408136 - Flags: review?(cpearce)
Attachment #8408131 - Attachment is obsolete: true
Attachment #8408131 - Flags: review?(cpearce)
Created attachment 8408138 [details] [diff] [review]
Part 2 - Change to stagefright demuxer;
Attachment #8408138 - Flags: review?(cpearce)
Attachment #8408136 - Attachment is obsolete: true
Attachment #8408136 - Flags: review?(cpearce)
Created attachment 8408139 [details] [diff] [review]
Part 5 - Remove Chrome demuxer;
Attachment #8408139 - Flags: review?(cpearce)
(Reporter)

Updated

3 years ago
Attachment #8408128 - Flags: review?(cpearce) → review+
(Reporter)

Comment 26

3 years ago
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-
(Reporter)

Updated

3 years ago
Attachment #8408139 - Flags: review?(cpearce) → review+
https://tbpl.mozilla.org/?tree=Try&rev=3a084b7ead8d
Attachment #8408139 - Attachment description: Part 3 - Remove Chrome demuxer; → Part 4 - Remove Chrome demuxer;
Created attachment 8419159 [details] [diff] [review]
Part 2 - Import libstagefright code from AOSP;
Attachment #8419159 - Flags: review?(cpearce)
Attachment #8408138 - Attachment is obsolete: true
Created attachment 8419163 [details] [diff] [review]
Part 3 - Change to stagefright demuxer;
Attachment #8419163 - Flags: review?(cpearce)
Created attachment 8419164 [details] [diff] [review]
Part 4 - Build config changes for libstagefright demuxer;
Attachment #8419164 - Flags: review?(mh+mozilla)
Attachment #8408139 - Attachment description: Part 4 - Remove Chrome demuxer; → Part 5 - Remove Chrome demuxer;
(Reporter)

Updated

3 years ago
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.
(Reporter)

Comment 33

3 years ago
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-
Created attachment 8419215 [details] [diff] [review]
Part 4 - Build config changes for libstagefright demuxer;
Attachment #8419215 - Flags: review?(mh+mozilla)
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+
Created attachment 8419227 [details] [diff] [review]
Part 3 - Change to stagefright demuxer;

Fixed all the review recommendations except for fast seeking which requires further investigation.
Attachment #8419227 - Flags: review?(cpearce)
Attachment #8419163 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=29a453c8f4dc
(Reporter)

Comment 38

3 years ago
(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;
}
Created attachment 8419779 [details] [diff] [review]
Part 3 - Change to stagefright demuxer;
Attachment #8419779 - Flags: review?(cpearce)
Attachment #8419227 - Attachment is obsolete: true
Attachment #8419227 - Flags: review?(cpearce)
(Reporter)

Comment 40

3 years ago
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-
Created attachment 8419920 [details] [diff] [review]
Part 3 - Change to stagefright demuxer;
Attachment #8419920 - Flags: review?(cpearce)
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.
(Reporter)

Comment 43

3 years ago
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/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
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
https://hg.mozilla.org/mozilla-central/rev/ca32138df109
https://hg.mozilla.org/mozilla-central/rev/05fe24d0499a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 8421347 [details] [diff] [review]
Part 6 - Fix build issues and Hf build
Attachment #8421347 - Flags: review?(cpearce)
(Reporter)

Updated

3 years ago
Attachment #8421347 - Flags: review?(cpearce) → review+
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
https://tbpl.mozilla.org/?tree=Try&rev=7025508241dc
(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
Flags: needinfo?(ajones)
(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)
Created attachment 8422882 [details] [diff] [review]
Part 6 - Fix build bustage and modify namespace
Attachment #8422882 - Flags: review?(cpearce)
Attachment #8421347 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=9515a165452b
(Reporter)

Updated

3 years ago
Attachment #8422882 - Flags: review?(cpearce) → review+
Created attachment 8424620 [details] [diff] [review]
Part 7 - Add conversion to Annex B
Attachment #8424620 - Flags: review?(cpearce)
(Reporter)

Comment 57

3 years ago
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?
Created attachment 8425089 [details] [diff] [review]
Part 7 - Add conversion to Annex B
Attachment #8425089 - Flags: review?(cpearce)
Attachment #8424620 - Attachment is obsolete: true
Attachment #8424620 - Flags: review?(cpearce)
(Reporter)

Comment 59

3 years ago
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-
Created attachment 8426000 [details] [diff] [review]
Part 7 - Add conversion to Annex B

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
(Reporter)

Comment 61

3 years ago
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://tbpl.mozilla.org/?tree=Try&rev=501379f5c312
https://hg.mozilla.org/integration/mozilla-inbound/rev/f75676ac4f7d
https://hg.mozilla.org/mozilla-central/rev/f75676ac4f7d
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Depends on: 1014613
Depends on: 1014626
Depends on: 1014794
No longer depends on: 1014794
Depends on: 1014814

Comment 65

3 years ago
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)

Comment 66

3 years ago
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.
(Reporter)

Comment 69

3 years ago
(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)

Updated

3 years ago
Depends on: 1068055
No longer depends on: 1068055
Depends on: 1153730

Updated

2 years ago
Depends on: 1144107

Updated

2 years ago
Depends on: 1179497
No longer depends on: 1179497
Flags: sec-review?(ptheriault)
You need to log in before you can comment on or make changes to this bug.