Closed Bug 1317239 Opened 8 years ago Closed 8 years ago

Config the codec if it supports adaptive playback feature for seamless change in resolution.

Categories

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

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Type:
defect

Tracking

(firefox52 wontfix, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

Details

Attachments

(2 files)

Bug 1317239 - Part1 - Add an API to ask if the codec support adaptive playback.
58 bytes, text/x-review-board-request
jhlin
: review+
Details
Bug 1317239 - Part2 - Config the video decoder with adaptive playback feature if it is supported.
58 bytes, text/x-review-board-request
jhlin
: review+
Details 
By document [1], we should ask if the codec supports this feature then set the MAX width height to MediaFormat. 

First we need an API to ask this feature then set the value.

Take [2] for reference.


[1] https://developer.android.com/about/versions/android-4.4.html#Multimedia
[2] http://androidxref.com/5.1.1_r6/xref/external/chromium_org/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java#509
Once we decide to change the behavior "do not recreate decoder when resolution change", I will start to work on this bug. Pause here.
Blocks: 1299105
Priority: -- → P1
Attachment #8813511 - Flags: review?(jolin)
Attachment #8813512 - Flags: review?(jolin)
Comment on attachment 8813511 [details]
Bug 1317239 - Part1 - Add an API to ask if the codec support adaptive playback.

https://reviewboard.mozilla.org/r/94940/#review95170

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/HardwareCodecCapabilityUtils.java:57
(Diff revision 1)
>      }
>      return false;
>    }
>  
> +  @WrapForJNI
> +  public static boolean checkSupportsAdaptivePlayback(String aMimeType) {

There could be more than one decoder component that support given MIME-type.
I think it's better to check the feature after decoder creation using its component name rather than just MIME-type.
Comment on attachment 8813511 [details]
Bug 1317239 - Part1 - Add an API to ask if the codec support adaptive playback.

https://reviewboard.mozilla.org/r/94940/#review95174

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/HardwareCodecCapabilityUtils.java:57
(Diff revision 1)
>      }
>      return false;
>    }
>  
> +  @WrapForJNI
> +  public static boolean checkSupportsAdaptivePlayback(String aMimeType) {

Sorry I meant using component name **and** MIME-type.
Comment on attachment 8813512 [details]
Bug 1317239 - Part2 - Config the video decoder with adaptive playback feature if it is supported.

https://reviewboard.mozilla.org/r/94942/#review95172

::: dom/media/platforms/android/MediaCodecDataDecoder.cpp:82
(Diff revision 1)
>      if (!mSurfaceTexture) {
>        NS_WARNING("Failed to create SurfaceTexture for video decode\n");
>        return InitPromise::CreateAndReject(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__);
>      }
>  
> +    // Check if the codec supports adaptive playback or not.

Suggest to check the feature after `InitDecoder` (where `MediaCodec` is created).
Maybe introduce a boolean data member to store it for later?
Attachment #8813512 - Flags: review?(jolin) → review-
Comment on attachment 8813511 [details]
Bug 1317239 - Part1 - Add an API to ask if the codec support adaptive playback.

https://reviewboard.mozilla.org/r/94940/#review95176
Attachment #8813511 - Flags: review?(jolin) → review-
Thank you for your suggestion.

Addressed, please review it again.

Thanks.
Comment on attachment 8813511 [details]
Bug 1317239 - Part1 - Add an API to ask if the codec support adaptive playback.

https://reviewboard.mozilla.org/r/94940/#review95190

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/HardwareCodecCapabilityUtils.java:66
(Diff revision 2)
> +          return false;
> +      }
> +
> +      try {
> +          MediaCodecInfo info = aCodec.getCodecInfo();
> +          if (info.isEncoder()) {

Nit: remove this check. In theory an encoder won't be passes as aCodec. If it were, cannot have this feature anyway.
Attachment #8813511 - Flags: review?(jolin) → review+
Comment on attachment 8813512 [details]
Bug 1317239 - Part2 - Config the video decoder with adaptive playback feature if it is supported.

https://reviewboard.mozilla.org/r/94942/#review95194

LGTM.
Attachment #8813512 - Flags: review?(jolin) → review+
Thank you for the quick review~~
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d45db027c09
Part1 - Add an API to ask if the codec support adaptive playback. r=jolin
https://hg.mozilla.org/integration/autoland/rev/2e9dbc8426fb
Part2 - Config the video decoder with adaptive playback feature if it is supported. r=jolin
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4d45db027c09
https://hg.mozilla.org/mozilla-central/rev/2e9dbc8426fb
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Uplifted to Aurora as part of the roll-up patch in bug 1299105.
https://hg.mozilla.org/releases/mozilla-aurora/rev/783ca8448b09
status-firefox52: --- → fixed
Depends on: 1360626
Depends on: 1363547
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: