1163486 - Remove MP4Reader

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

MP4Reader can be replaced by the MediaFormatReader and the MP4Demuxer. It can be removed

Comment 2

[Alfredo Yang (:alfredo)]

Attachment: remove_mp4_reader

Comment 3

[Jean-Yves Avenard [:jya]]

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

Comment 4

[Alfredo Yang (:alfredo)]

(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.

Comment 5

[Jean-Yves Avenard [:jya]]

(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

Comment 6

[Alfredo Yang (:alfredo)]

Attachment: remove_mp4_reader

Comment 7

[Alfredo Yang (:alfredo)]

Attachment: gtest_mp4demuxer

Comment 8

[Alfredo Yang (:alfredo)]

Attachment: gtest_mp4reader

Comment 9

[Alfredo Yang (:alfredo)]

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?

Comment 10

[Jean-Yves Avenard [:jya]]

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 #

Comment 11

[Alfredo Yang (:alfredo)]

(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

Comment 12

[Jean-Yves Avenard [:jya]]

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

Comment 13

[Jean-Yves Avenard [:jya]]

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.

Comment 14

[Bobby Holley (:bholley)]

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.

Comment 15

[Alfredo Yang (:alfredo)]

Attachment: gtest_mp4demuxer

Comment 16

[Bobby Holley (:bholley)]

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.

Comment 17

[Alfredo Yang (:alfredo)]

Attachment: remove_mp4_reader

Comment 18

[Alfredo Yang (:alfredo)]

Attachment: gtest_mp4demuxer

Comment 19

[Alfredo Yang (:alfredo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c590b64a21ce

Comment 20

[Carsten Book [:Tomcat]]

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 ?

Comment 21

[Alfredo Yang (:alfredo)]

Attachment: remove_mp4_reader

Rebase.

Comment 22

[Alfredo Yang (:alfredo)]

Attachment: gtest_mp4demuxer

Rebase.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1704ea727e81
https://hg.mozilla.org/integration/mozilla-inbound/rev/79619b679f82

Comment 24

[Carsten Book [:Tomcat]]

sorry had to back this out for b2g bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=11933980&repo=mozilla-inbound

Comment 25

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1e8846fda0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7064dafa286

Comment 26

[Alfredo Yang (:alfredo)]

Attachment: remove_mp4_reader

Fixed b2g emulator-l bustage.

Comment 27

[Alfredo Yang (:alfredo)]

Attachment: gtest_mp4demuxer

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f906e5bd227

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c88e335ddd5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/45c8fc442a14

Comment 29

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

https://hg.mozilla.org/mozilla-central/rev/c88e335ddd5e
https://hg.mozilla.org/mozilla-central/rev/45c8fc442a14