Closed Bug 1232223 Opened 4 years ago Closed 4 years ago

Remove MOZ_WEBM, MOZ_VPX, and MOZ_WAVE #ifdefs because they are always supported

Categories

(Core :: Audio/Video: Playback, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(5 files)

Part 2: Remove MOZ_VPX #ifdefs because VPx is always supported, though not necessarily enabled.
Attachment #8697911 - Flags: review?(jyavenard)
Part 3: Remove MOZ_WAVE #ifdefs because Wave is always supported.
Attachment #8697912 - Flags: review?(jyavenard)
I did not remove MOZ_WEBM_ENCODER because its value depends on whether we are linking with Vorbis or Tremor, which is controlled by MOZ_SAMPLE_TYPE_FLOAT32 (for non-Android and non-ARM platforms).
Priority: -- → P2
Comment on attachment 8697910 [details] [diff] [review]
remove-MOZ_WEBM.patch

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

LGTM, probably want someone more familiar with the build system though (glandium?). I don't understand the change in mozinfo.py in particular

::: config/external/moz.build
@@ +29,4 @@
>  if CONFIG['MOZ_WEBM_ENCODER']:
>      external_dirs += ['media/libmkv']
>  
>  if CONFIG['MOZ_VPX'] and not CONFIG['MOZ_NATIVE_LIBVPX']:

The title states removal of MOZ_VPX, so why no remove it all ?

it's always set to 1 below

::: configure.in
@@ +5141,5 @@
>    AC_DEFINE_UNQUOTED([ATTRIBUTE_ALIGNED_MAX],
>                       [${ac_cv_c_attribute_aligned}],[Maximum supported data alignment])
>  fi
>  
> +MOZ_VPX=1

this should go too no ?

::: dom/html/HTMLAudioElement.cpp
@@ +23,5 @@
>  NS_IMPL_NS_NEW_HTML_ELEMENT(Audio)
>  
>  namespace mozilla {
>  namespace dom {
>  

the removal of IsAudioAPIEnabled appears out of scope of the patch

::: dom/media/MediaDecoder.cpp
@@ +38,5 @@
>  static const int DEFAULT_HEURISTIC_DORMANT_TIMEOUT_MSECS = 60000;
>  
>  namespace mozilla {
>  
> +// The amount of instability we tolerate in calls to

out of scope
Attachment #8697910 - Flags: review?(jyavenard) → review+
Comment on attachment 8697911 [details] [diff] [review]
remove-MOZ_VPX.patch

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

oh I see, you're removing MOZ_VPX here.
Attachment #8697911 - Flags: review?(jyavenard) → review+
Comment on attachment 8697912 [details] [diff] [review]
remove-MOZ_WAVE.patch

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

LGTM, same comment as for patch 1
Attachment #8697912 - Flags: review?(jyavenard) → review+
Comment on attachment 8697910 [details] [diff] [review]
remove-MOZ_WEBM.patch

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

Mike, can you please review the build system changes to remove these unnecessary media #ifdefs?

::: dom/html/HTMLAudioElement.cpp
@@ +23,5 @@
>  NS_IMPL_NS_NEW_HTML_ELEMENT(Audio)
>  
>  namespace mozilla {
>  namespace dom {
>  

OK. I can move this to a separate patch. IsAudioAPIEnabled is not actually used or defined anywhere.

::: dom/media/MediaDecoder.cpp
@@ +38,5 @@
>  static const int DEFAULT_HEURISTIC_DORMANT_TIMEOUT_MSECS = 60000;
>  
>  namespace mozilla {
>  
> +// The amount of instability we tolerate in calls to

OK.
Attachment #8697910 - Flags: review?(mh+mozilla)
Comment on attachment 8697910 [details] [diff] [review]
remove-MOZ_WEBM.patch

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

I'm sure there are people who will complain about the removal of this option. Run this through dev-platform, maybe?

::: python/mozbuild/mozbuild/mozinfo.py
@@ -92,5 @@
>      d['tests_enabled'] = substs.get('ENABLE_TESTS') == "1"
>      d['bin_suffix'] = substs.get('BIN_SUFFIX', '')
>      d['addon_signing'] = substs.get('MOZ_ADDON_SIGNING') == '1'
> -
> -    d['webm'] = bool(substs.get('MOZ_WEBM'))

There are tests using the corresponding webm condition in dom/media/test/mochitest.ini
Attachment #8697910 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8697912 [details] [diff] [review]
remove-MOZ_WAVE.patch

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

::: python/mozbuild/mozbuild/mozinfo.py
@@ -91,5 @@
>      d['telemetry'] = substs.get('MOZ_TELEMETRY_REPORTING') == '1'
>      d['tests_enabled'] = substs.get('ENABLE_TESTS') == "1"
>      d['bin_suffix'] = substs.get('BIN_SUFFIX', '')
>      d['addon_signing'] = substs.get('MOZ_ADDON_SIGNING') == '1'
> -    d['wave'] = bool(substs.get('MOZ_WAVE'))

Same remark as for webm.
(In reply to Mike Hommey [:glandium] from comment #8)
> I'm sure there are people who will complain about the removal of this
> option. Run this through dev-platform, maybe?

The --disable-webm build is actually broken and, according to bug 964484, has been broken since bug 891705. I don't think anyone is going to miss --disable-webm. :)

> There are tests using the corresponding webm condition in
> dom/media/test/mochitest.ini

I will remove the webm and wave tests in a new patch.
Blocks: 964484
Part 4: Run webm tests unconditionally.
Attachment #8700281 - Flags: review?(jyavenard)
Part 5: Run wave tests unconditionally.
Attachment #8700282 - Flags: review?(jyavenard)
Attachment #8700281 - Flags: review?(jyavenard) → review+
Attachment #8700282 - Flags: review?(jyavenard) → review+
You need to log in before you can comment on or make changes to this bug.