1320618 - Check if codec supports adaptive playback to determine the decoder support recycling or not

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect, P1)

Description

[James Cheng[:JamesCheng]]

This bug is spawned by bug Bug 1299105 comment 34,

Since current checkSupportsAdaptivePlayback API[1] only allowed passing MediaCodec instance to ask...but for remote decoding design, we only have this instance on remote process(and in Java), it is not easy to handle in "C++" side... I should figure out more easy way to handle this, I don't want to add new interface on AIDL for IPC.

[1] checkSupportsAdaptivePlayback
http://searchfox.org/mozilla-central/rev/9a3ab17838f039aab023837d905115f8990e442e/mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/HardwareCodecCapabilityUtils.java#58

Comment 1

[James Cheng[:JamesCheng]]

Hi John,
I found that for local decoding case we create mediacodec by mimetype[1] but the mimeType is translated by "TranslateMimeType". For remote case, we used the mimetype extracted from MediaFormat.

Can we used the same approach(from MediaFormat) for both case? 

If so, I would like to change the API
"public static boolean checkSupportsAdaptivePlayback(MediaCodec aCodec, String aMimeType)" into
"public static boolean checkSupportsAdaptivePlayback(MediaFormat aFormat)" 

which can be used to both remote and local design to implement the check supporting API asking if the decoder supports recycling(requested by bug 1299105 comment 34 and comment 37).

Thanks.
[1]
http://searchfox.org/mozilla-central/rev/957458d8fa2328c2a760dbb30e7f1f1efa55b4d0/dom/media/platforms/android/MediaCodecDataDecoder.cpp#48

[2]
http://searchfox.org/mozilla-central/rev/9a3ab17838f039aab023837d905115f8990e442e/mobile/android/base/java/org/mozilla/gecko/media/Codec.java#281

Comment 2

[James Cheng[:JamesCheng]]

Attachment: Bug 1320618 - Check if codec supports adaptive playback to determine the decoder support recycling or not.

Review commit: https://reviewboard.mozilla.org/r/96224/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96224/

Comment 3

[James Cheng[:JamesCheng]]

Hi John, 
Please ignore comment 1 and help to review the part1 patch.

I will provide part2 patch like

Check support when I got MediaFormat instance
mIsSupportAdaptivePlayback =  java::HardwareCodecCapabilityUtils::CheckSupportsAdaptivePlayback(mFormat);
and 
bool SupportDecoderRecycling() const override
  {
   
    return mIsSupportAdaptivePlayback;
  }

if the part1 is being acceptable.

Thank you.

Comment 4

[James Cheng[:JamesCheng]]

Attachment: Bug 1320618-Part2-SupportDecoderRecycling return treu only when codec supports adaptive playback

Review commit: https://reviewboard.mozilla.org/r/96234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96234/

Comment 5

[John Lin [:jhlin][:jolin]]

Passing MediaFormat to a method named CheckSupportsAdaptivePlayback doesn't make sense to me and the patch doesn't address bug 1317239 comment 4. I suggest leaving it as is and do the checking in CodecProxy for remote decoder.

Comment 6

[John Lin [:jhlin][:jolin]]

Comment on attachment 8815201 [details]
Bug 1320618 - Check if codec supports adaptive playback to determine the decoder support recycling or not.

https://reviewboard.mozilla.org/r/96224/#review96666

Comment 7

[James Cheng[:JamesCheng]]

(In reply to John Lin [:jolin][:jhlin] from comment #5)
> Passing MediaFormat to a method named CheckSupportsAdaptivePlayback doesn't
> make sense to me and the patch doesn't address bug 1317239 comment 4. I
For solving bug 1317239 comment 4, I just want all logic use the same mimetype(From MediaFormat) and approximately get the same decoder for all callers. Anyway it is not robust.

> suggest leaving it as is and do the checking in CodecProxy for remote
> decoder.
IIUC, 
I should add a new interface in AIDL since MediaCodec instance only exists in decoder process(call it bool checkadaptive()).

And call remote.checkadaptive() after "configure" called[1]

Do you agree with adding a sync RPC call to for just asking if it supports adaptive playback?

[1]
http://searchfox.org/mozilla-central/rev/d98418da69edeb1f2f8e6f3840157fae1512f89b/mobile/android/base/java/org/mozilla/gecko/media/CodecProxy.java#110

Comment 8

[John Lin [:jhlin][:jolin]]

(In reply to James Cheng[:JamesCheng] from comment #7)
> (In reply to John Lin [:jolin][:jhlin] from comment #5)
> > Passing MediaFormat to a method named CheckSupportsAdaptivePlayback doesn't
> > make sense to me and the patch doesn't address bug 1317239 comment 4. I
> For solving bug 1317239 comment 4, I just want all logic use the same
> mimetype(From MediaFormat) and approximately get the same decoder for all
> callers. Anyway it is not robust.
> 
> > suggest leaving it as is and do the checking in CodecProxy for remote
> > decoder.
> IIUC, 
> I should add a new interface in AIDL since MediaCodec instance only exists
> in decoder process(call it bool checkadaptive()).

 Yes. isAdaptivePlaybackSupported() sounds more clear to me.

> 
> And call remote.checkadaptive() after "configure" called[1]

 Or introduce a new method CodecProxy.isAdaptivePlaybackSupported() that calls it, then overriding RemoteVideoDecoder::SupportDecoderRecycling() to call that.
 Also, cache the value in CodecProxy or RemoteVideoDecoder to avoid multiple IPC calls.
 
> 
> Do you agree with adding a sync RPC call to for just asking if it supports
> adaptive playback?
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> d98418da69edeb1f2f8e6f3840157fae1512f89b/mobile/android/base/java/org/
> mozilla/gecko/media/CodecProxy.java#110

 Sure.

Comment 9

[James Cheng[:JamesCheng]]

Thank you for your feedback.

I will provide a new patch for that!

Comment 10

[James Cheng[:JamesCheng]]

Comment on attachment 8815201 [details]
Bug 1320618 - Check if codec supports adaptive playback to determine the decoder support recycling or not.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96224/diff/1-2/

Comment 11

[John Lin [:jhlin][:jolin]]

Comment on attachment 8815201 [details]
Bug 1320618 - Check if codec supports adaptive playback to determine the decoder support recycling or not.

https://reviewboard.mozilla.org/r/96224/#review96982

LGTM. Please fix the issue in `JellyBeanAsyncCodec` before landing.

::: dom/media/platforms/android/MediaCodecDataDecoder.cpp:386
(Diff revision 2)
>                      MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__));
>      return NS_ERROR_FAILURE;
>    }
>  
>    // Check if the video codec supports adaptive playback or not.
> -  if (aSurface && java::HardwareCodecCapabilityUtils::CheckSupportsAdaptivePlayback(
> +  if (aSurface) {

Nit: this is for video decoder only and would be better to be put in VideoDataDecoder code rather than common init code.

::: dom/media/platforms/android/RemoteDataDecoder.cpp:284
(Diff revision 2)
>  
>    layers::ImageContainer* mImageContainer;
>    const VideoInfo& mConfig;
>    RefPtr<AndroidSurfaceTexture> mSurfaceTexture;
>    DurationQueue mInputDurations;
> +  bool mIsCodecSupportAdaptivePlayback = false;

Nit: or `Maybe<bool>` and lazily init the value in `SupportDecoderRecycling()`

::: mobile/android/base/java/org/mozilla/gecko/media/JellyBeanAsyncCodec.java:303
(Diff revision 2)
>  
>      @Override
>      public void configure(MediaFormat format, Surface surface, MediaCrypto crypto, int flags) {
>          assertCallbacks();
>  
>          // Video decoder should config with adaptive playback capability.

Please move this to `Codec.configure()` to avoid (future) duplicating code in other `AsyncCodec` subclasses.

Comment 12

[James Cheng[:JamesCheng]]

Comment on attachment 8815201 [details]
Bug 1320618 - Check if codec supports adaptive playback to determine the decoder support recycling or not.

https://reviewboard.mozilla.org/r/96224/#review96982

> Nit: this is for video decoder only and would be better to be put in VideoDataDecoder code rather than common init code.

But the mFormat need to be set before the MeidaCodec configure called, and checking the adaptive playback API needs MediaCodec instance...
So it seems I need to put the check in this section.

> Nit: or `Maybe<bool>` and lazily init the value in `SupportDecoderRecycling()`

will do it, thanks.

> Please move this to `Codec.configure()` to avoid (future) duplicating code in other `AsyncCodec` subclasses.

But in Codec.configure, I don't have MediaCodec instance, I can only do it in the JellyBeanAsyncCodec.configure. 
Did I misunderstand anything?

Comment 13

[John Lin [:jhlin][:jolin]]

Comment on attachment 8815201 [details]
Bug 1320618 - Check if codec supports adaptive playback to determine the decoder support recycling or not.

https://reviewboard.mozilla.org/r/96224/#review96982

> But in Codec.configure, I don't have MediaCodec instance, I can only do it in the JellyBeanAsyncCodec.configure. 
> Did I misunderstand anything?

What I meant is adding a method `AsyncCodec.isAdaptivePlaybackSupported` which JellyBeanAsyncCodec implements and it calls `CheckSupportsAdaptivePlayback()`. `Codec.configure()` then inserts max width/height.

Comment 14

[John Lin [:jhlin][:jolin]]

Comment on attachment 8815201 [details]
Bug 1320618 - Check if codec supports adaptive playback to determine the decoder support recycling or not.

https://reviewboard.mozilla.org/r/96224/#review96982

> But the mFormat need to be set before the MeidaCodec configure called, and checking the adaptive playback API needs MediaCodec instance...
> So it seems I need to put the check in this section.

Yes. It will require rewriting `VideoDataDecoder::Init()` and `MediaCodecDataDecoder::InitDecoder()`. This is just a nit so I'm okay if you don't change it.

Comment 15

[James Cheng[:JamesCheng]]

Comment on attachment 8815201 [details]
Bug 1320618 - Check if codec supports adaptive playback to determine the decoder support recycling or not.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96224/diff/2-3/

Comment 16

[James Cheng[:JamesCheng]]

Comment on attachment 8815201 [details]
Bug 1320618 - Check if codec supports adaptive playback to determine the decoder support recycling or not.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96224/diff/3-4/

Comment 17

[James Cheng[:JamesCheng]]

Thank you for review.

Attach try result 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6723fa0fb06fbc39d6b71b52e66f616df8afdfbc

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/803eb84eb9a6
Check if codec supports adaptive playback to determine the decoder support recycling or not. r=jolin

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/803eb84eb9a6

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Uplifted to Aurora as part of the roll-up patch in bug 1299105.
https://hg.mozilla.org/releases/mozilla-aurora/rev/783ca8448b09