784082 - Disable non-mti codecs in media/webrtc

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Ralph Giles (:rillian)]

Currently we're building the default set of codecs supported by the media/webrtc library. Instead we want to support only the minimal set of codecs.

For sudio: G711 (PCMU & PCMA) and Opus.
For video: VP8.

Comment 1

[Ralph Giles (:rillian)]

This is just updating the codec_database.h lists and gyp files based on MOZILLA_BUILD/build_with_mozilla.

Comment 2

[Ralph Giles (:rillian)]

Why did you close this bug? It's not resolved as far as I know.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Ralph Giles (:rillian) from comment #2)
> Why did you close this bug? It's not resolved as far as I know.

Oh. I thought this was flagged as finished in triage. Maybe a mistaken flag on my part?

Comment 4

[Ralph Giles (:rillian)]

Or someone else's. :)

In any case, please include a commit reference if you think something is resolved but left open.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Disable codecs we don't use, and make codec selection a command-line param to gyp

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 657619 [details] [diff] [review]
Disable codecs we don't use, and make codec selection a command-line param to gyp

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

Thanks for working on this. Just one comment from my work-in-progress. I hadn't gotten to the gyp part yet.

::: media/webrtc/trunk/src/engine_configurations.h
@@ +35,5 @@
>  // #define WEBRTC_CODEC_ISACFX  // fix-point iSAC implementation
>  #endif
>  #define WEBRTC_CODEC_AVT
>  
>  #ifndef WEBRTC_CHROMIUM_BUILD

I have

#if !defined(WEBRTC_CHROMIUM_BUILD) && !defined(WEBRTC_CHROMIUM_BUILD)

here in the upstream patch, which should make the #if 0 unnecessary.

Comment 7

[Ralph Giles (:rillian)]

(In reply to Ralph Giles (:rillian) from comment #6)

> #if !defined(WEBRTC_CHROMIUM_BUILD) && !defined(WEBRTC_CHROMIUM_BUILD)
#if !defined(WEBRTC_CHROMIUM_BUILD) && !defined(WEBRTC_MOZILLA_BUILD)

is what I meant. Sorry, bedtime patch review. :)

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Disable codecs we don't use, and make codec selection a command-line param to gyp

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 657726 [details] [diff] [review]
Disable codecs we don't use, and make codec selection a command-line param to gyp

Addressed your comment by removing them.  They're all set by configuring codecs in the call to gyp

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 657726 [details] [diff] [review]
Disable codecs we don't use, and make codec selection a command-line param to gyp

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

It would be nice to know what upstream thinks of this approach, but I have no objection if you remove the commented-out source lines.

Would it be cleaner to define which codecs are enabled for each build somewhere like audio_coding_module.gypi (or modules.gyp) based on build_with_mozilla, etc. instead of passing them on the gyp command line? That would let us maintain our codec list in the webrtc sub-tree, so it's visible upstream, and maintain the upstream and chromium codec selections in the same place rather than requiring they change their invocations as well.

::: media/webrtc/trunk/src/build/common.gypi
@@ +198,5 @@
> +      }],
> +      ['codec_isac_enable!=0', {
> +        'defines': [
> +          'WEBRTC_CODEC_ISAC',
> +#          'WEBRTC_CODEC_ISACFX',

This breaks the WEBRTC_ARCH_ARM conditional which chooses between these in engine_configurations.h. You probably want to duplicate that here to make the patch acceptable upstream.

::: media/webrtc/trunk/src/modules/audio_coding/main/source/audio_coding_module.gypi
@@ +101,5 @@
> +#        'acm_amr.h',
> +#        'acm_amrwb.cc',
> +#        'acm_amrwb.h',
> +#        'acm_celt.cc',
> +#        'acm_celt.h',

If these are replaced by the conditionals above, please just remove them, rather than commenting them out here and below.

::: media/webrtc/trunk/src/modules/audio_coding/neteq/neteq.gypi
@@ +83,5 @@
>          'webrtc_neteq.c',
>        ],
>      },
>    ], # targets
>    # Exclude the test target when building with chromium.

Please update the comment as well.

Comment 11

[(not currently active) Ted Mielczarek]

Comment on attachment 657726 [details] [diff] [review]
Disable codecs we don't use, and make codec selection a command-line param to gyp

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

Ok with the concept, but I don't like the execution.

::: configure.in
@@ +8750,5 @@
>  
>  if test -n "$MOZ_WEBRTC"; then
>     AC_MSG_RESULT("generating WebRTC Makefiles...")
>  
> +   WEBRTC_CONFIG="-D build_with_mozilla=1 -D include_internal_video_render=0 -D clang_use_chrome_plugins=0 -D enable_protobuf=0 -D codec_g711_enable=1 -D codec_opus_enable=1 -D codec_g722_enable=0 -D codec_ilbc_enable=0 -D codec_isac_enable=0 -D codec_pcm16b_enable=0 -D include_pulse_audio=0"

This is getting pretty big. Any chance we can stick this all into a mozilla-settings.gypi somehow and just include that on the commandline?

@@ +8752,5 @@
>     AC_MSG_RESULT("generating WebRTC Makefiles...")
>  
> +   WEBRTC_CONFIG="-D build_with_mozilla=1 -D include_internal_video_render=0 -D clang_use_chrome_plugins=0 -D enable_protobuf=0 -D codec_g711_enable=1 -D codec_opus_enable=1 -D codec_g722_enable=0 -D codec_ilbc_enable=0 -D codec_isac_enable=0 -D codec_pcm16b_enable=0 -D include_pulse_audio=0"
> +
> +   AC_MSG_RESULT("WEBRTC_CONFIG = ${WEBRTC_CONFIG}")

Seems overly verbose, I'd leave this out.

@@ +8756,5 @@
> +   AC_MSG_RESULT("WEBRTC_CONFIG = ${WEBRTC_CONFIG}")
> +
> +   GYP_WEBRTC_OPTIONS="--format=mozmake ${WEBRTC_CONFIG} ${EXTRA_GYP_DEFINES} --depth=${srcdir}/media/webrtc/trunk --toplevel-dir=${srcdir} -G OBJDIR=${_objdir}"
> +
> +   AC_MSG_RESULT("GYP_WEBRTC_OPTIONS = ${GYP_WEBRTC_OPTIONS}")

Same here.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Disable codecs we don't use, and move webrtc gyp options to a file

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 662386 [details] [diff] [review]
Disable codecs we don't use, and move webrtc gyp options to a file

I think you'll like this approach a lot better

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 662386 [details] [diff] [review]
Disable codecs we don't use, and move webrtc gyp options to a file

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

Much nicer, thanks!

::: media/webrtc/shared_libs.mk
@@ +48,5 @@
>    $(call EXPAND_LIBNAME_PATH,nrappkit,$(DEPTH)/media/mtransport/third_party/nrappkit/nrappkit_nrappkit) \
>    $(NULL)
> +
> +# If you enable one of these codecs in webrtc_config.gypi, you'll need to re-add the 
> +# relevant library from this list:

This is a little gross, but I guess we don't expect to be fiddling this very often? (Also you have a trailing space on the first line of the comment.)

::: media/webrtc/trunk/src/build/common.gypi
@@ +198,5 @@
> +      }],
> +      ['codec_isac_enable!=0', {
> +        'defines': [
> +          'WEBRTC_CODEC_ISAC',
> +#          'WEBRTC_CODEC_ISACFX',

You should probably just remove this instead of commenting it out.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Ted Mielczarek [:ted] from comment #14)

> Much nicer, thanks!

Yeah, much better than hiding all the webrtc config options in configure

> ::: media/webrtc/shared_libs.mk
> @@ +48,5 @@
> >    $(call EXPAND_LIBNAME_PATH,nrappkit,$(DEPTH)/media/mtransport/third_party/nrappkit/nrappkit_nrappkit) \
> >    $(NULL)
> > +
> > +# If you enable one of these codecs in webrtc_config.gypi, you'll need to re-add the 
> > +# relevant library from this list:
> 
> This is a little gross, but I guess we don't expect to be fiddling this very
> often? (Also you have a trailing space on the first line of the comment.)

Very rarely.  It was more to show what should go there, since it's somewhat non-obvious.

> ::: media/webrtc/trunk/src/build/common.gypi
> @@ +198,5 @@
> > +      }],
> > +      ['codec_isac_enable!=0', {
> > +        'defines': [
> > +          'WEBRTC_CODEC_ISAC',
> > +#          'WEBRTC_CODEC_ISACFX',
> 
> You should probably just remove this instead of commenting it out.

Actually I just commented it - it was in engine_configurations.h and I moved it here, and it was disabled with a comment in engine_configurations, so I've kept it here in a disabled state.  Now looks like:
      ['codec_isac_enable!=0', {
        'defines': [
          'WEBRTC_CODEC_ISAC',
# enable this instead for fixed-point iSAC
#          'WEBRTC_CODEC_ISACFX',
        ],
      }],

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Landed in 8b19e0980250 on alder on 9/19/2012, later merged to m-c