Make the gstreamer media backend play whitelisted codecs only

RESOLVED FIXED in mozilla23

Status

()

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: alessandro.d, Assigned: alessandro.d)

Tracking

Trunk
mozilla23
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Comment 1

6 years ago
Posted file implement whitelist (obsolete) —
Attachment #727519 - Flags: review?(cpearce)
(Assignee)

Updated

6 years ago
Blocks: 794282
(Assignee)

Comment 2

6 years ago
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.
Attachment #727519 - Attachment is obsolete: true
Attachment #727519 - Flags: review?(cpearce)
(Assignee)

Updated

6 years ago
Attachment #727525 - Flags: review?(cpearce)
(Assignee)

Comment 3

6 years ago
Posted patch fix mp4 caps (obsolete) — Splinter Review
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"},
Attachment #727525 - Attachment is obsolete: true
Attachment #727525 - Flags: review?(cpearce)
Attachment #727564 - Flags: review?(chris.double)
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!
Attachment #727564 - Flags: feedback+
(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

6 years ago
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;
Attachment #727564 - Flags: review?(chris.double) → review+
(Assignee)

Comment 7

6 years ago
Attachment #727564 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2209aa5267f5
Assignee: nobody → alessandro.d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Comment 10

6 years ago
I don't suppose there is a pref here to enable playback of non-whitelisted codecs? :(
There is not. That's sort of the point. :/
Depends on: 1149376
You need to log in before you can comment on or make changes to this bug.