1036775 - Make B2G Platform Decoder Module Work for MSE only

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Blake Wu [:bwu][:blakewu]]

The platform decoder module in bug 941302 is initially designed for MSE. 
If it is enabled, it will conflict with current MediaOmxDecoder/Reader.
Need to find a way to make it work for MSE only.

Comment 1

[Ivan Tsay (:ITsay)]

This is part of feature work.

Comment 2

[Kai-Chih Hu [:khu]]

Based on the discussion with the partner, agreed to remove the feature-b2g flag on this.

Comment 3

[Blake Wu [:bwu][:blakewu]]

Attachment: Bug-1036775-Add-one-more-define-check-to-separate-omx-decoder-mp4-decoder.patch

Check if MOZ_OMX_DECODER is defined or not. If yes, don't use MOZ_FMP4. 
This patch is based on the premise that MSE directly creates MP4Reader (http://dxr.mozilla.org/mozilla-central/source/content/media/mediasource/MediaSourceReader.cpp#338) instead of creating it via DecoderTraits::CreateReader.

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8481133 [details] [diff] [review]
Bug-1036775-Add-one-more-define-check-to-separate-omx-decoder-mp4-decoder.patch

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

::: content/media/DecoderTraits.cpp
@@ +404,5 @@
>      codecList = gWebMCodecs;
>      result = CANPLAY_MAYBE;
>    }
>  #endif
> +#if !defined(MOZ_OMX_DECODER) && defined(MOZ_FMP4)

The |!defined(MOZ_OMX_DECODER)| check should be moved to IsMP4SupportedType(), with a comment about why it's there and that it will be removed.

You should also file a bug to remove this check once we're ready to move to the PDM for all MP4 media on B2G. Put that bug number in the comment as well.

@@ +409,2 @@
>    if (MP4Decoder::CanHandleMediaType(nsDependentCString(aMIMEType),
>                                       aRequestedCodecs)) {

This guard should be changed to IsMP4SupportedType(), and IsMP4SupportedType changed to optionally take a codecs argument.

Comment 5

[Blake Wu [:bwu][:blakewu]]

Attachment: Bug-1036775-Add-one-more-define-check-to-separate-omx-decoder-mp4-decoderV2.patch

1. Some changes according to comment 4. 
2. Create a bug (bug 1061034) to further enable all MP4s playback with MOZ_FMP4

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8482099 [details] [diff] [review]
Bug-1036775-Add-one-more-define-check-to-separate-omx-decoder-mp4-decoderV2.patch

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

Good! :)

::: content/media/DecoderTraits.cpp
@@ +325,2 @@
>  {
> +// Currently on B2G, FMP4 is only working for MSE playback

Note here that MSE does its mimetype checks elsewhere. Also, sentences end with a period.

@@ +418,1 @@
>                                       aRequestedCodecs)) {

nit: indentation needs to be fixed here.

Comment 7

[Marvin Khoo [:Marvin_Khoo]]

This is part of the dependencies for supporting MSE on B2G for 2.1.

Comment 8

[Blake Wu [:bwu][:blakewu]]

Attachment: Bug-1036775-Add-one-more-define-check-to-separate-omx-decoder-mp4-decoder-Final.patch

Carry r+ from edwin.

Comment 9

[Blake Wu [:bwu][:blakewu]]

TBPL: https://tbpl.mozilla.org/?tree=Try&rev=ed0337cff731

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f630cf7cc5ad

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f630cf7cc5ad

Comment 12

[Blake Wu [:bwu][:blakewu]]

[Blocking Requested - why for this release]:

Comment 13

[Blake Wu [:bwu][:blakewu]]

This bug is for feature 2.1, but missed the train to 2.1 branch out.

Comment 14

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8482444 [details] [diff] [review]
Bug-1036775-Add-one-more-define-check-to-separate-omx-decoder-mp4-decoder-Final.patch

Approval Request Comment
[Feature/regressing bug #]:
 2.1
[User impact if declined]:
 Impact Bug 1027519 [Meta][User story] Support MSE on Firefox OS (B2G)
[Describe test coverage new/current, TBPL]:
 https://hg.mozilla.org/mozilla-central/rev/f630cf7cc5ad 
[Risks and why]: 
 Missed the train to 2.1 branch out.
[String/UUID change made/needed]:

Comment 15

[Eric Chou [:ericchou] [:echou]]

Confirmed with Kevin Hu and he said we only need feature-b2g + approval to get the patch merged into aurora. No need to be blocking-b2g + approval. So clear the nomination.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ffb144a500a4