853306 - Make the gstreamer media backend play whitelisted codecs only

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Alessandro Decina]

As discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=794282, the gstreamer backend needs to support whitelisted codecs only. Patch already feedback+'d in https://bugzilla.mozilla.org/show_bug.cgi?id=794282#c60

Comment 1

[Alessandro Decina]

Attachment: implement whitelist

Comment 2

[Alessandro Decina]

Attachment: ignore metadata demuxers in whitelist checks

Same patch with... 

+              if (strstr (klass, "Demuxer") && !strstr(klass, "Metadata"))

...a check added to skip metadata parser elements while looking for container elements in the pipeline.

Fixes regression with mp3s with id3 tags.

Comment 3

[Alessandro Decina]

Attachment: fix mp4 caps

Hrm, sorry for the noise with the previous patches. This is the final patch, promised. Fixes m4a tests with:

-  {"audio/x-m4a", "audio/mpeg, mpegversion=(int)4"},
+  {"audio/x-m4a", "audio/x-m4a"},

Comment 4

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

Comment on attachment 727564 [details] [diff] [review]
fix mp4 caps

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

Confirming that tests pass for me on Linux with this patch and the one from bug 853306 applied. Thanks Alessandro!

Comment 5

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

(In reply to Chris Pearce (:cpearce) from comment #4)
> Confirming that tests pass for me on Linux with this patch and the one from
> bug 853306 applied.

I mean bug 853325... ;)

Comment 6

[cajbir (:cajbir)]

Comment on attachment 727564 [details] [diff] [review]
fix mp4 caps

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

Just minor style changes. r+ with those.

::: content/media/gstreamer/GStreamerFormatHelper.cpp
@@ +53,5 @@
>  GStreamerFormatHelper::GStreamerFormatHelper()
>    : mFactories(nullptr),
>      mCookie(static_cast<uint32_t>(-1))
>  {
> +  unsigned int i;

Remove this and declare in each for statement.

@@ +56,5 @@
>  {
> +  unsigned int i;
> +
> +  mSupportedContainerCaps = gst_caps_new_empty();
> +  for (i = 0; i < G_N_ELEMENTS(mContainers); i++) {

for(unsigned int i = 0; ...

@@ +63,5 @@
> +    gst_caps_append(mSupportedContainerCaps, caps);
> +  }
> +
> +  mSupportedCodecCaps = gst_caps_new_empty();
> +  for (i = 0; i < G_N_ELEMENTS(mCodecs); i++) {

for(unsigned int i = 0; ...

::: content/media/gstreamer/GStreamerReader.cpp
@@ +296,5 @@
>        break;
>      }
>    }
>  
> +  if (!NS_FAILED(ret))

Use NS_SUCCEEDED

@@ +353,5 @@
> +  GstElement* element;
> +  GstElementFactory* factory;
> +  const char* klass;
> +  GstCaps* caps;
> +  GstPad* pad;

Declare these on first use/initialization where possible.

@@ +356,5 @@
> +  GstCaps* caps;
> +  GstPad* pad;
> +
> +  GstIterator *it = gst_bin_iterate_recurse(GST_BIN(mPlayBin));
> +  GstIteratorResult res;

Declare this inside the loop at first use.

@@ +357,5 @@
> +  GstPad* pad;
> +
> +  GstIterator *it = gst_bin_iterate_recurse(GST_BIN(mPlayBin));
> +  GstIteratorResult res;
> +  while(!done) {

Space needed between 'while' keyword and '('.

@@ +358,5 @@
> +
> +  GstIterator *it = gst_bin_iterate_recurse(GST_BIN(mPlayBin));
> +  GstIteratorResult res;
> +  while(!done) {
> +    res = gst_iterator_next(it, (void **)&element);

so this becomes:
  while (!done) {
    GstIteratorResult res = gst_iterator_next...

@@ +399,5 @@
> +  }
> +
> +  if (unsupported)
> +    return NS_ERROR_FAILURE;
> +  return NS_OK;

Replace the conditional/return with:

  return unsupported ? NS_ERROR_FAILURE : NS_OK;

Comment 7

[Alessandro Decina]

Attachment: fixed review comments

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2209aa5267f5

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2209aa5267f5

Comment 10

[Chris Hubick]

I don't suppose there is a pref here to enable playback of non-whitelisted codecs? :(

Comment 11

[Ralph Giles (:rillian)]

There is not. That's sort of the point. :/