Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: jya, Assigned: ayang)

Tracking

(Blocks: 1 bug)

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected, firefox42 fixed)

Details

Attachments

(2 attachments, 9 obsolete attachments)

(Reporter)

Description

4 years ago
MP4Reader can be replaced by the MediaFormatReader and the MP4Demuxer. It can be removed
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1165775
(Assignee)

Comment 2

4 years ago
Posted patch remove_mp4_reader (obsolete) — Splinter Review
Assignee: nobody → ayang
Attachment #8625296 - Flags: review?(jyavenard)
(Reporter)

Comment 3

4 years ago
Comment on attachment 8625296 [details] [diff] [review]
remove_mp4_reader

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

there's more that can now be removed in media/libstagefright/binding and the MP4Demuxer.

::: dom/media/MediaFormatReader.h
@@ +29,5 @@
>    typedef media::Interval<int64_t> ByteInterval;
>  
>  public:
>    explicit MediaFormatReader(AbstractMediaDecoder* aDecoder,
> +                             MediaDataDemuxer* aDemuxer = nullptr,

MediaFormatReader has no reason to ever be used without a demuxer it's the primary concept on why MediaFormatReader exists.

The test TestMediaFormatReader.cpp should create a demuxer.

::: dom/media/fmp4/MP4Decoder.cpp
@@ +28,5 @@
>  
>  MediaDecoderStateMachine* MP4Decoder::CreateStateMachine()
>  {
> +  MediaDecoderReader* reader =
> +    static_cast<MediaDecoderReader*>(new MediaFormatReader(this, new MP4Demuxer(GetResource())));

you don't need the static_cast anymore, it was only required due to the use of expr ? a : b

::: dom/media/fmp4/MP4Demuxer.cpp
@@ +20,5 @@
> +  if (!log) {
> +    log = PR_NewLogModule("MP4Demuxer");
> +  }
> +  return log;
> +}

The various PDM have references to this GetDemuxerLog, this is wrong. PDM should have nothing to do with MP4Demuxer

This can be done in a follow up bug

::: dom/media/gtest/TestMediaFormatReader.cpp
@@ +26,5 @@
>  
>    explicit TestBinding(const char* aFileName = "gizmo.mp4")
>      : decoder(new MP4Decoder())
>      , resource(new MockMediaResource(aFileName))
> +    , reader(new MediaFormatReader(decoder))

not sure i understand the point of those tests nor this change...

MediaFormatReader can't work without a demuxer, it will immediately assert upon use.

You need to create a MP4Demuxer first, and pass it to MediaFormatReader.

::: dom/media/gtest/moz.build
@@ +9,5 @@
>      'TestAudioCompactor.cpp',
>      'TestGMPCrossOrigin.cpp',
>      'TestGMPRemoveAndDelete.cpp',
>      'TestIntervalSet.cpp',
> +    'TestMediaFormatReader.cpp',

We aren't testing the media format reader in this test, but the MP4 demuxer really. as such the name of the test should reflect that

::: dom/media/mediasource/MediaSourceReader.cpp
@@ +688,5 @@
>    if ((aType.LowerCaseEqualsLiteral("video/mp4") ||
>         aType.LowerCaseEqualsLiteral("audio/mp4")) &&
>        MP4Decoder::IsEnabled() && aDecoder) {
> +    MediaDecoderReader* reader =
> +      static_cast<MediaDecoderReader*>(new MediaFormatReader(aDecoder, new MP4Demuxer(aDecoder->GetResource()), aBorrowedTaskQueue));

no need for static_cast

::: dom/media/platforms/wmf/WMFAudioMFTManager.h
@@ +7,5 @@
>  #if !defined(WMFAudioOutputSource_h_)
>  #define WMFAudioOutputSource_h_
>  
>  #include "WMF.h"
> +#include "MediaFormatReader.h"

no need.

::: dom/media/platforms/wmf/WMFMediaDataDecoder.h
@@ +8,5 @@
>  #define WMFMediaDataDecoder_h_
>  
>  
>  #include "WMF.h"
> +#include "MediaFormatReader.h"

there should be no need for those includes.

::: dom/media/platforms/wmf/WMFVideoMFTManager.h
@@ +7,5 @@
>  #if !defined(WMFVideoMFTManager_h_)
>  #define WMFVideoMFTManager_h_
>  
>  #include "WMF.h"
> +#include "MediaFormatReader.h"

no need
Attachment #8625296 - Flags: review?(jyavenard) → review-
(Assignee)

Comment 4

4 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #3)
> Comment on attachment 8625296 [details] [diff] [review]
> remove_mp4_reader
> 
> Review of attachment 8625296 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> there's more that can now be removed in media/libstagefright/binding and the
> MP4Demuxer.

I'll remove MP4TrackDemuxer.h and mp4_demuxer.h as well, other files need to be removed?

> 
> ::: dom/media/gtest/moz.build
> @@ +9,5 @@
> >      'TestAudioCompactor.cpp',
> >      'TestGMPCrossOrigin.cpp',
> >      'TestGMPRemoveAndDelete.cpp',
> >      'TestIntervalSet.cpp',
> > +    'TestMediaFormatReader.cpp',
> 
> We aren't testing the media format reader in this test, but the MP4 demuxer
> really. as such the name of the test should reflect that

I'll remove TestMP4Demuxer.cpp and TestMP4Reader.cpp. Let me know if you want to keep them.
(Reporter)

Comment 5

4 years ago
(In reply to Alfredo Yang from comment #4)
> (In reply to Jean-Yves Avenard [:jya] from comment #3)
> > Comment on attachment 8625296 [details] [diff] [review]
> > remove_mp4_reader
> > 
> > Review of attachment 8625296 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > there's more that can now be removed in media/libstagefright/binding and the
> > MP4Demuxer.
> 
> I'll remove MP4TrackDemuxer.h and mp4_demuxer.h as well, other files need to
> be removed?
> 
> > 
> > ::: dom/media/gtest/moz.build
> > @@ +9,5 @@
> > >      'TestAudioCompactor.cpp',
> > >      'TestGMPCrossOrigin.cpp',
> > >      'TestGMPRemoveAndDelete.cpp',
> > >      'TestIntervalSet.cpp',
> > > +    'TestMediaFormatReader.cpp',
> > 
> > We aren't testing the media format reader in this test, but the MP4 demuxer
> > really. as such the name of the test should reflect that
> 
> I'll remove TestMP4Demuxer.cpp and TestMP4Reader.cpp. Let me know if you
> want to keep them.

We can keep them, adapting it to use MediaFormatReader and MP4Demuxer (the MediaDataDemuxer one) would add one line of code.. it's fairly simple.

Forgot also to comment that we can remove the mediareader / mp4 related prefs in all.js
(Assignee)

Comment 6

4 years ago
Posted patch remove_mp4_reader (obsolete) — Splinter Review
Attachment #8625296 - Attachment is obsolete: true
Attachment #8625876 - Flags: review?(jyavenard)
(Assignee)

Comment 7

4 years ago
Posted patch gtest_mp4demuxer (obsolete) — Splinter Review
Attachment #8625877 - Flags: review?(jyavenard)
(Assignee)

Comment 8

4 years ago
Posted patch gtest_mp4reader (obsolete) — Splinter Review
(Assignee)

Comment 9

4 years ago
I got following error in TestMP4Reader:

2015-06-24 21:16:27.170033 UTC - 1943143168[1003dd070]: Loaded VideoDecodeAcceleration framework.
2015-06-24 21:16:27.170434 UTC - 1943143168[1003dd070]: Loaded CoreMedia framework.
2015-06-24 21:16:27.171415 UTC - 1943143168[1003dd070]: Loaded VideoToolbox framework.
[40366] WARNING: MediaFormatReader without a decoder owner, can't get HWAccel: file /Users/Alfredo/mozilla/mozilla-central/dom/media/MediaFormatReader.cpp, line 162
Hit MOZ_CRASH() at /Users/Alfredo/mozilla/mozilla-central/dom/media/MediaDecoderReader.h:168

I don't have idea how to deal it. Any suggestion?
Flags: needinfo?(jyavenard)
(Reporter)

Comment 10

4 years ago
Comment on attachment 8625876 [details] [diff] [review]
remove_mp4_reader

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

::: dom/media/webm/IntelWebMVideoDecoder.cpp
@@ +20,5 @@
>  #include "vpx/vpx_decoder.h"
>  
>  #undef LOG
>  PRLogModuleInfo* GetDemuxerLog();
>  #define LOG(...) MOZ_LOG(GetDemuxerLog(), mozilla::LogLevel::Debug, (__VA_ARGS__))

Side note: Using a demuxer log for the media data decoder is counter-intuitive. decoder logs should be moved into their own categories. need a new bug #
Attachment #8625876 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 11

4 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #10)
> Comment on attachment 8625876 [details] [diff] [review]
> remove_mp4_reader
> 
> Review of attachment 8625876 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/webm/IntelWebMVideoDecoder.cpp
> @@ +20,5 @@
> >  #include "vpx/vpx_decoder.h"
> >  
> >  #undef LOG
> >  PRLogModuleInfo* GetDemuxerLog();
> >  #define LOG(...) MOZ_LOG(GetDemuxerLog(), mozilla::LogLevel::Debug, (__VA_ARGS__))
> 
> Side note: Using a demuxer log for the media data decoder is
> counter-intuitive. decoder logs should be moved into their own categories.
> need a new bug #

Thanks for review, bug 1177256
(Reporter)

Comment 12

4 years ago
Comment on attachment 8625877 [details] [diff] [review]
gtest_mp4demuxer

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

passing on to bholley for review, I wonder if he has some fancy ideas on how to simulate synchronous calls with media promises.

::: dom/media/gtest/TestMP4Demuxer.cpp
@@ +8,4 @@
>  #include "MP4Stream.h"
> +#include "MediaTaskQueue.h"
> +#include "MediaPromise.h"
> +#include "MediaDataDemuxer.h"

you shouldn't need those two headers (MediaPromise and MediaDataDemuxer) they come with MP4Demuxer.h (which returns arguments that are mediapromise)

@@ +62,5 @@
> +
> +    WaitForPromise();
> +
> +    mVideoTrack = mDemuxer->GetTrackDemuxer(TrackInfo::kVideoTrack, 0);
> +    mAudioTrack = mDemuxer->GetTrackDemuxer(TrackInfo::kAudioTrack, 1);

the tracknumber is of the type only. so it should be 0 if you only have one audio track. 1 would mean the second audio track.

@@ +144,5 @@
> +  template<typename FunctionType>
> +  void
> +  RunOnTaskQueue(FunctionType aFun)
> +  {
> +    nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(aFun);

this should be a nsRefPtr
Attachment #8625877 - Flags: review?(jyavenard) → review?(bobbyholley)
(Reporter)

Comment 13

4 years ago
Comment on attachment 8625878 [details] [diff] [review]
gtest_mp4reader

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

due to bug 1175752, this shouldn't work.. though we can fix that in that bug instead.
(Reporter)

Updated

4 years ago
Flags: needinfo?(jyavenard)
Comment on attachment 8625877 [details] [diff] [review]
gtest_mp4demuxer

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

::: dom/media/gtest/TestMP4Demuxer.cpp
@@ +84,5 @@
> +  {
> +    InitializeWaiting();
> +
> +    RunOnTaskQueue(
> +      [aTrackDemuxer, this] () {

Capturing raw refcounted types is forbidden and will break static analysis. Do something like the following:

nsRefPtr<MediaTrackDemuxer> trackDemuxer = aTrackDemuxer;
nsRefPtr<MP4DemuxerBinding> self = this;
[trackDemuxer, self] ...

@@ +144,5 @@
> +  template<typename FunctionType>
> +  void
> +  RunOnTaskQueue(FunctionType aFun)
> +  {
> +    nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(aFun);

Actually not - we use nsCOMPtr for things that inherit nsISupports.

@@ +161,5 @@
> +    mWait = false;
> +    mon.NotifyAll();
> +  }
> +
> +  void WaitForPromise()

Instead of doing this stuff, we should rewrite the logic to be asynchronous and use promises. See TestMediaPromise for an example of what that looks like.
Attachment #8625877 - Flags: review?(bobbyholley) → review-
(Assignee)

Comment 15

4 years ago
Posted patch gtest_mp4demuxer (obsolete) — Splinter Review
Attachment #8625877 - Attachment is obsolete: true
Attachment #8625878 - Attachment is obsolete: true
Attachment #8628647 - Flags: review?(bobbyholley)
(Assignee)

Updated

4 years ago
Blocks: 1146086
Comment on attachment 8628647 [details] [diff] [review]
gtest_mp4demuxer

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

I'm really sorry this took so long - it's a pretty difficult patch to review, and I haven't had a lot of uninterrupted time.

I didn't read everything closely, but here are some high-level comments. Please address them and then feel free to land without re-review.

::: dom/media/gtest/TestMP4Demuxer.cpp
@@ +43,5 @@
>      EXPECT_EQ(NS_OK, resource->Open(nullptr));
>    }
>  
> +  template<typename Function>
> +  void StartTest(const Function& aFunction)

I would call this RunTestAndWait

@@ +48,5 @@
> +  {
> +    mDemuxer->Init()->Then(mTaskQueue, __func__,
> +      [aFunction] () {
> +        aFunction();
> +      },

Why not just pass aFunction here? Is the type inference from the template messing you up? You could also do:

Function func(aFunction); And then pass func or Move(func).

Though it looks like the callers are actually just passing lambda literals. So you could just make this take Function&& instead, which should pass just fine to ->Then.

@@ +54,5 @@
> +    mTaskQueue->AwaitShutdownAndIdle();
> +  }
> +
> +  template<typename Function>
> +  void CheckTrackKeyFrame(MediaTrackDemuxer* aTrackDemuxer, const Function& aFunction)

Instead of having this method take a callback, have it return a promise (perhaps a GenericPromise?).

@@ +55,5 @@
> +  }
> +
> +  template<typename Function>
> +  void CheckTrackKeyFrame(MediaTrackDemuxer* aTrackDemuxer, const Function& aFunction)
> +  {

Please assert the task queue that this is on - it runs on the binding task queue, right?

@@ +73,5 @@
> +      aFunction();
> +      return;
> +    }
> +
> +    RunOnTaskQueue(

Why does this need to be inside RunOnTaskQueue? Aren't we already on that task queue?

@@ +93,5 @@
> +  }
> +
> +  template<typename Function>
> +  void CheckTrackSamples(MediaTrackDemuxer* aTrackDemuxer, const Function& aFunction)
> +  {

Same design comments as CheckTrackKeyframe.
Attachment #8628647 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 17

4 years ago
Posted patch remove_mp4_reader (obsolete) — Splinter Review
Attachment #8625876 - Attachment is obsolete: true
Attachment #8628647 - Attachment is obsolete: true
Attachment #8635789 - Flags: review+
(Assignee)

Comment 18

4 years ago
Posted patch gtest_mp4demuxer (obsolete) — Splinter Review
Attachment #8635790 - Flags: review+
this failed to apply - patching file dom/media/mediasource/MediaSourceReader.cpp
Hunk #1 FAILED at 14
Hunk #2 succeeded at 698 with fuzz 2 (offset 1 lines).
1 out of 2 hunks FAILED -- saving rejects to file dom/media/mediasource/MediaSourceReader.cpp.rej
patching file modules/libpref/init/all.js
Hunk #1 FAILED at 468
1 out of 1 hunks FAILED -- saving rejects to file modules/libpref/init/all.js.rej

maybe need a rebase ? can you take a look ?
Flags: needinfo?(ayang)
Keywords: checkin-needed
(Assignee)

Comment 21

4 years ago
Posted patch remove_mp4_reader (obsolete) — Splinter Review
Rebase.
Attachment #8635789 - Attachment is obsolete: true
Attachment #8635790 - Attachment is obsolete: true
Flags: needinfo?(ayang)
Attachment #8636372 - Flags: review+
(Assignee)

Comment 22

4 years ago
Posted patch gtest_mp4demuxer (obsolete) — Splinter Review
Rebase.
Attachment #8636373 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 26

4 years ago
Fixed b2g emulator-l bustage.
Attachment #8636372 - Attachment is obsolete: true
Attachment #8636373 - Attachment is obsolete: true
Attachment #8636509 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c88e335ddd5e
https://hg.mozilla.org/mozilla-central/rev/45c8fc442a14
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1290644
You need to log in before you can comment on or make changes to this bug.