760140 - AAC and MP3 not supported in <audio> (but AAC supported as a <video> sound track!) when the Fluendo Complete Codec Pack is installed

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Henri Sivonen (:hsivonen)]

Steps to reproduce:
1) Build Firefox with --enable-gstreamer
2) Have the Fluendo Complete Codec Pack aka. "fluendo-plugins" and the "bad" codec set installed but don't have the "ugly", "ffmpeg" or separate "fluendo-mp3" GStreamer codecs installed.
3) Load http://hsivonen.iki.fi/test/moz/audio/aac.html
4) May be reload
5) Click the play button on the controller
6) Now try http://hsivonen.iki.fi/test/moz/audio/mp3.html
7) Navigate to http://html5test.com/

Expected results:
Expected canPlayType() to claim support for MP3 and AAC as tested by http://html5test.com/ . Expected the built-in controller to appear without reload on aac.html and mp3.html and expected clicking the play button to play audio on both pages.

Actual results:
Behaves as if MP3 and AAC were not supported. Yet, AAC audio plays on http://hsivonen.iki.fi/test/moz/video-selection/src-mp4.html

Comment 1

[cajbir (:cajbir)]

Oddly the MP3 and AAC doesn't playback on B2G using the patch in bug 714408 either.

Comment 2

[Alessandro Decina]

Attachment: Add audio/mpeg and audio/mp4 to the supported types

Comment 3

[Henri Sivonen (:hsivonen)]

The patch doesn't appear to query GStreamer for the availability of MP3 and AAC decoders. Am I missing something?

(As a follow-up, if an AAC decoder is present, it would probably be worthwhile to make canPlayType() respond "probably" when queried with the appropriate codecs parameter.)

Comment 4

[Alessandro Decina]

The patch doesn't query GStreamer because that's not how we usually do it in gst land. What we do instead is let the pipeline start decoding, possibly emit missing-plugin messages and handle those from the application.

For example when using totem under linux, if you try to decode something for which you're missing a decoder, totem will pop up an helper app that will request you to install the required package for your distro. If the installation is successful, totem will then continue decoding.

Unless someone is against this approach, I think that we should claim that we support the common formats (h264, aac, mp3 etc) and handle missing-plugin messages to install the required elements.

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to Alessandro Decina from comment #4)
> Unless someone is against this approach, I think that we should claim that
> we support the common formats (h264, aac, mp3 etc) and handle missing-plugin
> messages to install the required elements.

If a Web site offers content in H.264, AAC or MP3 unconditionally (i.e. <video src=foo.mp4>, <audio src=bar.m4a> or <audio src=baz.mp3>), I think it makes sense to pop up the Gnome missing codec wizard.

However, the point of the canPlayType() API is querying the current capabilities of the browser, so I think the responses should be based on probing GStreamer to see if the relevant codecs are installed at the time of the query. This would be consistent with how canPlayType() works in IE and Safari: in IE and Safari, canPlayType() does not reflect the potential of the media framework but the current configuration of the media framework.

Likewise, when there are multiple source elements, the choice between them should depend on the currently installed capabilities so
<video>
<source src=foo.mp4 type='video/mp4; codecs="avc1.42E01E, mp4a.40.2"'>
<source src="foo.webm" type='video/webm; codecs="vp8, vorbis"'>
</video>
should play the WebM version without popping up the Gnome missing codec wizard if H.264 and AAC support isn't present.

Comment 6

[Alessandro Decina]

Attachment: Query the GstRegistry for the required demuxers/decoders from canPlayType

This patch implements looking at the GstRegistry to check if elements to demux/decode the formats passed to canPlayType() are actually installed.

Comment 7

[Henri Sivonen (:hsivonen)]

Comment on attachment 633952 [details] [diff] [review]
Query the GstRegistry for the required demuxers/decoders from canPlayType

I'm unsetting the review request, because I'm not a reviewer for this module. I'll try to give review comments anyway, but for landing, this needs to be reviewed from someone from the video team, e.g. Chris Double.

>   static CanPlayStatus CanHandleMediaType(const char* aMIMEType,
>-                                          char const *const ** aSupportedCodecs);
>+                                          const char* aCodecs,
>+                                          char const *const ** aSupportedCodecs,
>+                                          bool * aCheckSupportedCodecs);

bool* aCheckSupportedCodecs.

>+                                       bool *aCheckCodecList)

For consistency with the rest of Gecko video code, please don't use the space after the type name before the pointer * and instead put the space after.

>+bool nsGStreamerDecoder::CanHandleMediaType(const char* aMIMEType,
>+                                            const char* aCodecs) {

In .cpp files, please put the method return type on a line of its own before the method name.

>+    mCookie(-1)

Here, you assign a negative value to an unsigned variable.

>+{
>+  const char *containers[4][2] = {
>+    {"video/mp4", "video/quicktime"},
>+    {"video/quicktime", "video/quicktime"},

In the interest of avoiding format proliferation, I think Firefox should not support video/quicktime, since IE and Chrome don't support it.

>+    {"audio/mp4", "audio/mpeg, mpegversion=(int)4"},
>+    {"audio/mpeg", "audio/mpeg, mpegversion=(int)1"},
>+  };
>+  memcpy(mContainers, containers, sizeof(containers));

Why are these arrays copied to the heap instead of using static const globals instead of object members for these arrays?

>+  const char *codecs[6][2] = {
>+    {"avc1.42E01E", "video/x-h264"},

This combination of AVC levels and profiles is particular important support, since this is the value that Dive into HTML5 advertises for testing H.264 support, however…

>+    {"avc1.42001E", "video/x-h264"},
>+    {"avc1.58A01E", "video/x-h264"},
>+    {"avc1.4D401E", "video/x-h264"},
>+    {"avc1.64001E", "video/x-h264"},

…where do these other AVC level and profile combinations come from? Do we know that these particular combinations make sense to support and that GStreamer either doesn't support other combinations are supporting other combinations would be bad for the Web? That is, why aren't we wildcarding the part after "avc1."?

>+  if (caps == NULL)
>+    return false;

(and elsewhere)
Please use
if (!caps) {
  return false;
}
i.e. no == with NULL and braces even around one-line blocks.

>+  nsCCharSeparatedTokenizer tokenizer(codecs, ',');

Does this correctly ignore optional spaces?

>+  GList *factories = GetFactories();
>+
>+  GList *list;
>+  /* here aCaps contains [containerCaps, [codecCaps1, [codecCaps2, ...]]] so process
>+   * caps structures individually as we want one element for _each_
>+   * structure */
>+  for (unsigned int i = 0; i < gst_caps_get_size(aCaps); i++) {
>+    GstStructure *s = gst_caps_get_structure(aCaps, i);
>+    GstCaps *caps = gst_caps_new_full(gst_structure_copy(s), NULL);
>+    list = gst_element_factory_list_filter (factories, caps, GST_PAD_SINK, FALSE);
>+    gst_caps_unref(caps);
>+    if (list == NULL)
>+      return false;
>+    g_list_free(list);
>+  }

I'm not competent to review the above bit, but I trust it uses GStreamer correctly.

>+    uint32_t mCookie;

This does not compile, because the type isn't defined. You need #include "mozilla/Types.h"

Higher-level comments:

When JavaScript calls canPlayType with specific codecs and those codecs are present, with this patch applied Firefox answers "maybe". It would be better to answer "probably" in that case, since the convention is to reply "probably" when specific codecs were queried for and it was possible to make the query all the way to the media back end.

On the other hand, from code inspection, it seems that if the user has the MP4 demuxer installed but does not have H.264 or AAC decoders installed, querying canPlayType for "video/mp4" results in "maybe" even though none of the whitelisted codecs are present, so no video/mp4 file would actually play. I think this is a problem, because the MP4 demuxer is in the "good" plug-in package, so it's quite likely that there will be a bunch of users who have the demuxer without the codecs installed.

To solve this problem, when canPlayType is called with 'video/mp4' without the codecs parameter, I think Firefox should query GStreamer for H.264/AAC/MP4 and reply "maybe" only if H.264, AAC and MP4 are all supported and "" otherwise. And when canPlayType is called with audio/mp4, I think Firefox should query GStreamer for AAC/MP4 and reply "maybe" only if both AAC and MP4 are supported. Likewise, when queried for audio/mpeg, I think Firefox should query for the MP3 decoder in addition to the stream demuxer.

There's also a special case that needs attention: audio/mpeg; codecs="mp3". To my knowledge "mp3" is a bogus value for the codecs parameter, but IE (both 9 and 10) responds "probably" to canPlayType('audio/mpeg; codecs="mp3"') and Chrome and Safari reply "maybe". Since the other browsers that support MP3 decoding response something other than "", I think it would be bad for us to reply "" when MP3 decoding supported. I suggest we treat "mp3" as a codec name we recognize (like IE) and respond "probably" if MP3 decoding is present. See http://hsivonen.iki.fi/test/moz/canplaytype.html

(I haven't tested if other browsers support MP3 audio in a MP4 container. I think we shouldn't support it unless either 1) all of Chrome, Safari and IE support it or 2) we need it for Shumway. canPlayType results suggest that Chrome and IE support it but Safari doesn't, but I don't have an actual test file to see if it actually plays.)

Thank you for working on this!

Comment 8

[Henri Sivonen (:hsivonen)]

Oh, and we really need unit tests for this stuff.

Comment 9

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #7)
> When JavaScript calls canPlayType with specific codecs and those codecs are
> present, with this patch applied Firefox answers "maybe". It would be better
> to answer "probably" in that case, since the convention is to reply
> "probably" when specific codecs were queried for and it was possible to make
> the query all the way to the media back end.

Furthermore, it appears that to score on html5test.com, the browser needs to answer "probably" in this case.

Comment 10

[Paul Adenot (:padenot)]

Attachment: Fix compilation error.

The patch does not compile on my machine (linux, clang). The attached patch fixes the build error.

Comment 11

[Landry Breuil (:gaston)]

Stumbled upon that issue when testing the gstreamer backend. Note that at the moment, youtube.com/html5 says firefox with gstreamer backend doesnt support h264 because the JS tests for two codecs, one of them missing atm ;:

    if (!videoElement.canPlayType ||
        !videoElement.canPlayType('video/mp4; codecs="avc1.42001E, mp4a.40.2"')) {
      var cH264 = document.getElementById('c-h264');
      cH264.className = cH264.className.replace(/\bsuccess\b/, 'error');
    }

I can easily reproduce it in the web console:

[13:36:16.210] document.createElement('video').canPlayType('video/mp4; codecs="mp4a.40.2"')
[13:36:16.232] "probably"

[13:36:50.710] document.createElement('video').canPlayType('video/mp4; codecs="avc1.42001E, mp4a.40.2"')
[13:36:50.721] ""

Looking forward to get that patch commited (since it adds the missing avc profile), is there something we can do about it ?

Comment 12

[cajbir (:cajbir)]

Comment on attachment 635913 [details] [diff] [review]
Fix compilation error.

Review wasn't requested but I'll provide it to help move this bug along. I assume this will be integrated into the original patch that Henri provided feedback on.

>+#include <prtypes.h>

Use <mozilla/Types.h>

>-    uint32_t mCookie;
>+    PRUint32 mCookie;

Leave this as uint32_t.

Comment 13

[cajbir (:cajbir)]

Comment on attachment 633952 [details] [diff] [review]
Query the GstRegistry for the required demuxers/decoders from canPlayType

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

I agree with Henri's comments in comment 7 and comment 9. Other than that, just the deleting of the static singleton mentioned below.

::: content/media/gstreamer/nsGStreamerFormatHelper.cpp
@@ +15,5 @@
> +
> +nsGStreamerFormatHelper *nsGStreamerFormatHelper::Instance() {
> +  if (gInstance == NULL) {
> +    gst_init(NULL, NULL);
> +    gInstance = new nsGStreamerFormatHelper();

Unless I missed it this will need to be deleted'd somewhere otherwise it'll be picked up as a leak in the memory reporting tools. One approach is to do it in nsLayoutStatics::Shutdown (in layout/build/nsLayoutStatics.cpp). You'll see some #ifdef'd MOZ_MEDIA code in that file already as a guide.

Comment 14

[Wolfgang Rosenauer [:wolfiR]]

I'm using this patch locally and adressed some of the review comments.
I'm not sure what to do exactly with comment 13 but would love to get this into the tree. Anyone up for help?

Comment 15

[Tobias Netzel]

(In reply to Wolfgang Rosenauer [:wolfiR] from comment #14)
> I'm using this patch locally and adressed some of the review comments.
> I'm not sure what to do exactly with comment 13 but would love to get this
> into the tree. Anyone up for help?

Unfortunately I didn't notice your comment and did pretty much the same thing and also addressed comment 13. I'll post my patch now.

Comment 16

[Tobias Netzel]

Attachment: update and preparing for review

Comment 17

[cajbir (:cajbir)]

Comment on attachment 683932 [details] [diff] [review]
update and preparing for review

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

Unfortunately the media code has been refactored somewhat since this patch was done. Mostly it's just miner class renaming but there's been movement in the codec detection side of things in bug 814284 which affects that area of this patch. I've reviewed the portions of this not related to that. If you rebase on those recent changes and fix up the points mentioned it should be good.

::: content/html/content/public/nsHTMLMediaElement.h
@@ +261,5 @@
>    // in *aSupportedCodecs. This list should not be freed, it is static data.
>    static CanPlayStatus CanHandleMediaType(const char* aMIMEType,
> +                                          const char* aCodecs,
> +                                          char const *const ** aSupportedCodecs,
> +                                          bool * aCheckSupportedCodecs);

bool* aCheckSupportedCodecs

::: content/media/gstreamer/GStreamerDecoder.cpp
@@ +17,5 @@
>  }
>  
> +bool
> +GStreamerDecoder::CanHandleMediaType(const char* aMIMEType,
> +                                       const char* aCodecs)

Align with const char* parameter above.

::: content/media/gstreamer/GStreamerFormatHelper.cpp
@@ +48,5 @@
> +};
> +
> +GStreamerFormatHelper::GStreamerFormatHelper()
> +  : mFactories(NULL),
> +    mCookie(-1)

Don't assign a negative value to an unsigned variable.

@@ +58,5 @@
> +    g_list_free(mFactories);
> +}
> +
> +bool GStreamerFormatHelper::CanHandleMediaType(const char* aMIMEType,
> +                                                 const char *aCodecs) {

Align "const char" with the line above it. And make it "const char* aCodecs". Same for the other functions that do this.

@@ +85,5 @@
> +  }
> +
> +  if (!capsString) {
> +    /* we couldn't find any matching caps */
> +    return NULL;

Use nullptr instead of NULL in all cases where NULL is used.

::: content/media/gstreamer/GStreamerFormatHelper.h
@@ +9,5 @@
> +
> +#include <gst/gst.h>
> +#include <mozilla/Types.h>
> +
> +class GStreamerFormatHelper {

Add a comment explaining what this class is for.

@@ +26,5 @@
> +                                  const char *aCodecs);
> +    char * const *CodecListFromCaps(GstCaps *aCaps);
> +    bool HaveElementsToProcessCaps(GstCaps *aCaps); 
> +    GList *GetFactories(); 
> +

Remove trailing white space on above two lines.

@@ +32,5 @@
> +
> +    static char const *const mContainers[4][2];
> +    static char const *const mCodecs[8][2];
> +    GList *mFactories;
> +    uint32_t mCookie;

Add comments to members explaining what they are.

Comment 18

[Tobias Netzel]

Unfortunately I don't have the time to build and test with current trunk sources; my base is Aurora. So for now I'll just fix up the mentioned points.
Anybody interested may feel free to get this updated.

Comment 19

[Tobias Netzel]

Attachment: applies to Aurora 19 only

This addresses all the points indicated from the review.
In case nobody integrates it into the current trunk sources I myself will work on this again after Aurora 20 is created.

Comment 20

[Tobias Netzel]

Attachment: update to Aurora 20

So here now an updated version that works in Aurora 20, hoping there haven't been other things braking this again.

Comment 21

[Cork]

(In reply to Tobias Netzel from comment #20)
> Created attachment 703840 [details] [diff] [review]
> update to Aurora 20
> 
> So here now an updated version that works in Aurora 20, hoping there haven't
> been other things braking this again.

You can't write a patch against aurora and ask for commit to central, as it's almost guaranteed that it doesn't apply.

patching file content/media/DecoderTraits.h
Hunk #1 FAILED at 48
1 out of 1 hunks FAILED
patching file layout/build/nsLayoutStatics.cpp
Hunk #1 FAILED at 77
1 out of 2 hunks FAILED

Comment 22

[Alessandro Decina]

Attachment: Query the GstRegistry for the required demuxers/decoders from canPlayType

Tobias: thanks for your help with this patch.

I now rebased it on top of m-c and made sure all the canPlayType tests pass.

Note that this patch is applied on top of my patches for bugs https://bugzilla.mozilla.org/show_bug.cgi?id=833628 and  https://bugzilla.mozilla.org/show_bug.cgi?id=829653

If you use git, you can pull this branch https://github.com/alessandrod/mozilla-central/commits/bugs/760140

Comment 23

[Tobias Netzel]

Good that you've picked it up again, because I won't be working on it again for at least the next two months.

Comment 24

[(no longer active)]

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

Comment 25

[Ed Morley [:emorley]]

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

Comment 26

[Florian Bender]

Is this Linux-only or does h.264, MP3 and AAC now work on Mac, too?

Comment 27

[cajbir (:cajbir)]

It may be possible to compile it with gstreamer on the Mac but I don't think it has had much testing there. Linux is the primary platform for this backend. Native Mac support is in development.

Comment 28

[Alessandro Decina]

Oh, I never tested this on linux. I work on Mac and wrote the backend on Mac. Works great :)

Comment 29

[cajbir (:cajbir)]

(In reply to Alessandro Decina from comment #28)
> Oh, I never tested this on linux. I work on Mac and wrote the backend on
> Mac. Works great :)

Even better!

Comment 30

[Florian Bender]

Thanks, great. Do you know if GStreamer is using AV Foundation on Mac, or if there is a translation layer for GStreamer i/f to AV Foundation backend available? If so, it wouldn't even be necessary to do all the Mac-only work (where AV Foundation is intended to be used, see Bug 801521). Otherwise we should investigate if AV Foundation provides any benefits GStreamer cannot (e. g. latency, leveraging the hw at its fullest and best?). We may eventually be able to combine all the backends of the different platforms and use a single interface for interacting with them (which means less maintenance work and easier and better interop).

Comment 31

[Alessandro Decina]

I don't know what's the state of #801521, I'd be all for using GStreamer on Mac. On Mac gst uses a lower level API called VideoToolBox, which is what is used by AVFoundation under the hood. At the time the gst plugin was written, AVFoundation didn't expose all the necessary interfaces to implement a gst plugin on top of it. Since last year or so it does, and I have some uncommitted code that uses it that I could get in shape and merge.

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

Bugger me. Maybe we can ship gstreamer-core with Firefox on Mac with the GStreamer plugin that uses VideoToolbox. Then we don't need to write another AVFoundation backend.

Comment 33

[Florian Bender]

Let's continue this discussion in Bug 851290.