Closed Bug 1036775 Opened 6 years ago Closed 6 years ago

Make B2G Platform Decoder Module Work for MSE only

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
2.1 S2 (15aug)
feature-b2g 2.1
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: bwu, Assigned: bwu)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Depends on: 941302
Blocks: MSE-FxOS
This is part of feature work.
feature-b2g: --- → 2.1
Assignee: nobody → bwu
Target Milestone: --- → 2.1 S2 (15aug)
Based on the discussion with the partner, agreed to remove the feature-b2g flag on this.
feature-b2g: 2.1 → ---
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.
Attachment #8481133 - Flags: review?(edwin)
Blocks: 1060900
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.
Attachment #8481133 - Flags: review?(edwin) → review-
1. Some changes according to comment 4. 
2. Create a bug (bug 1061034) to further enable all MP4s playback with MOZ_FMP4
Attachment #8481133 - Attachment is obsolete: true
Attachment #8482099 - Flags: review?(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.
Attachment #8482099 - Flags: review?(edwin) → review+
This is part of the dependencies for supporting MSE on B2G for 2.1.
feature-b2g: --- → 2.1
Carry r+ from edwin.
Attachment #8482099 - Attachment is obsolete: true
Attachment #8482444 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/f630cf7cc5ad
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.1?
This bug is for feature 2.1, but missed the train to 2.1 branch out.
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]:
Attachment #8482444 - Flags: approval-mozilla-aurora?
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.
blocking-b2g: 2.1? → ---
Attachment #8482444 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.