813645 - Move libyuv to be a normal third party media library so it can be used by others.

Status: RESOLVED FIXED

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

Description

[Paul Adenot (:padenot)]

The code is in media/webrtc/trunk/third_party/libyuv and should be in media/libyuv, if it is of interest for the media code.

This bug is about the build system bits.

Comment 1

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

CC-ing some possibly-relevant people to see how we should prioritize it.  I'll note you likely can access it from libxul code directly, but you'll then be dependent on --enable-webrtc being the default (which is is on desktop (only) - for now).

Upstream (indirect via webrtc) is here: http://code.google.com/p/libyuv/ :

"libyuv is an open source project that includes YUV scaling and conversion functionality.

Scale YUV to prepare content for compression, with point, bilinear or box filter.
Converts webcam formats to YUV (I420).
Convert YUV to formats for rendering/effects.
Rotate by 90 degrees to adjust for mobile devices in portrait mode.

Optimized for SSE2/SSSE3 on x86/x64.
Optimized for Neon on Arm. "

Changelog shows lots of work on NEON optimizations recently.

Comment 2

[Timothy B. Terriberry (:derf)]

See also bug 791941.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Part 1. Move libyuv into gkmedias

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Part 2. Always build libyuv even if webrtc is disabled

Comment 5

[Makoto Kato [:m_kato]]

also, https://code.google.com/p/libyuv/issues/detail?id=252 for upstream to export MJpegDecoder that uses WebRTC.

Comment 6

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

Comment on attachment 779599 [details] [diff] [review]
Part 1. Move libyuv into gkmedias

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

::: media/libyuv/include/libyuv/mjpeg_decoder.h
@@ +40,5 @@
>  // format, especially in Logitech devices. This class implements a decoder for
>  // MJPEG frames.
>  //
>  // See http://tools.ietf.org/html/rfc2435
> +class LIBYUV_API MJpegDecoder {

Not needed if we don't define SHARED_LIBRARY

::: media/libyuv/libyuv.gyp
@@ +37,5 @@
>          }],
>        ],
>        'defines': [
>          'HAVE_JPEG',
> +        'LIBYUV_BUILDING_SHARED_LIBRARY',

Why was this changed?  If it's so libyuv (in gkmedia) can be accessed from libxul on windows, this is normally handled via a whitelist of functions in symbols.def.in.  Is it in gkmedia on windows with this change?  Right now it's in libxul with the rest of webrtc.

Comment 7

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

Note: I have a patch to update libyuv to rev 719, I may update that before landing.  It's in bug 880419.  Note that that update shouldn't block this, and this doesn't block that one.

Comment 8

[Makoto Kato [:m_kato]]

(In reply to Randell Jesup [:jesup] from comment #6)
> Comment on attachment 779599 [details] [diff] [review]
> Part 1. Move libyuv into gkmedias
> 
> Review of attachment 779599 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libyuv/include/libyuv/mjpeg_decoder.h
> @@ +40,5 @@
> >  // format, especially in Logitech devices. This class implements a decoder for
> >  // MJPEG frames.
> >  //
> >  // See http://tools.ietf.org/html/rfc2435
> > +class LIBYUV_API MJpegDecoder {
> 
> Not needed if we don't define SHARED_LIBRARY
> 
> ::: media/libyuv/libyuv.gyp
> @@ +37,5 @@
> >          }],
> >        ],
> >        'defines': [
> >          'HAVE_JPEG',
> > +        'LIBYUV_BUILDING_SHARED_LIBRARY',
> 
> Why was this changed?  If it's so libyuv (in gkmedia) can be accessed from
> libxul on windows, this is normally handled via a whitelist of functions in
> symbols.def.in.  Is it in gkmedia on windows with this change?  Right now
> it's in libxul with the rest of webrtc.

OK. I use symbols.def instead of building as shared.

Comment 9

[Makoto Kato [:m_kato]]

Attachment: Part 1. Move libyuv into gkmedias v2

Comment 10

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

Comment on attachment 780210 [details] [diff] [review]
Part 1. Move libyuv into gkmedias v2

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

r+ with the dependency conditional change.

::: media/webrtc/trunk/peerconnection.gyp
@@ -38,5 @@
>  #          'webrtc/modules/modules.gyp:video_render_module',
>  #          'webrtc/system_wrappers/source/system_wrappers.gyp:system_wrappers',
>            'webrtc/video_engine/video_engine.gyp:video_engine_core',
>            'webrtc/voice_engine/voice_engine.gyp:voice_engine_core',
> -          '<(DEPTH)/third_party/libyuv/libyuv.gyp:libyuv',

make this dependency conditional on build_libyuv

::: media/webrtc/trunk/webrtc/common_video/common_video.gyp
@@ +101,5 @@
>          },
>        ],  # targets
>      }],  # include_tests
>    ],
> +}

If possible, don't make unnecessary changes to an import; it makes future merges more likely to conflict

Comment 11

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

Attachment: Move libyuv to media/libyuv

builds, now depends on bug 880419

Comment 12

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

Comment on attachment 8361311 [details] [diff] [review]
Move libyuv to media/libyuv

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

glandium and I guessed that using '.' would work (and it does); perhaps there's a better way

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8361311 [details] [diff] [review]
Move libyuv to media/libyuv

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

I'm going to redirect this to glandium.

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8361311 [details] [diff] [review]
Move libyuv to media/libyuv

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

::: build/gyp.mozbuild
@@ +22,5 @@
>      'use_system_libvpx': 0,
>      'build_libjpeg': 0,
>      'build_libvpx': 0,
> +    'build_libyuv': 0,
> +    'libyuv_dir': '$(topsrcdir)/media/libyuv',

Looks like you tried something here, and failed to make it actually work.

Try /media/libyuv without topsrcdir, and try not changing common_video.gyp

Comment 15

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

(In reply to Mike Hommey [:glandium] from comment #14)
> Comment on attachment 8361311 [details] [diff] [review]
> Move libyuv to media/libyuv
> 
> Review of attachment 8361311 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/gyp.mozbuild
> @@ +22,5 @@
> >      'use_system_libvpx': 0,
> >      'build_libjpeg': 0,
> >      'build_libvpx': 0,
> > +    'build_libyuv': 0,
> > +    'libyuv_dir': '$(topsrcdir)/media/libyuv',
> 
> Looks like you tried something here, and failed to make it actually work.
> 
> Try /media/libyuv without topsrcdir, and try not changing common_video.gyp

That generates 

-I/home/jesup/src/mozilla/inbound/media/webrtc/trunk/webrtc/common_video/media/libyuv/include

(wrong) and you're right, $(topsrcdir)/media/libyuv was an attempt to fix it by using an absolute path, that fails too with

-I/home/jesup/src/mozilla/inbound/media/webrtc/trunk/webrtc/common_video//home/jesup/src/mozilla/inbound/media/libyuv/include

libyuv_dir is how it's supposed to be done, but it looks like it's not handling absolute paths.  And $(DEPTH)/media/libyuv almost works, but it goes one level too high up which screws us.

This works, though it's not ideal:
    'libyuv_dir': '../../../../../media/libyuv',
That works, though only because it's being used from one point.  I'll go with that for now.

There are also some changes to moz.build to avoid unified build issues (static inlines of an _Abs() function.)  (Not sure why they didn't show up before...  But they're fixed now.  Maybe because I'd been doing incremental builds before.)

Comment 16

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

Attachment: Move libyuv to media/libyuv

Comment 17

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

Comment on attachment 8363669 [details] [diff] [review]
Move libyuv to media/libyuv

As mentioned, depends on the update to libyuv

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8363669 [details] [diff] [review]
Move libyuv to media/libyuv

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

Note I get this when applying:
media/webrtc/trunk/third_party/libyuv/Android.mk not tracked!
media/webrtc/trunk/third_party/libyuv/LICENSE_THIRD_PARTY not tracked!
media/webrtc/trunk/third_party/libyuv/OWNERS not tracked!
media/webrtc/trunk/third_party/libyuv/all.gyp no tracked!

::: build/gyp.mozbuild
@@ +22,5 @@
>      'use_system_libvpx': 0,
>      'build_libjpeg': 0,
>      'build_libvpx': 0,
> +    'build_libyuv': 0,
> +    'libyuv_dir': '../../../../../media/libyuv',

A local test shows this yields:
   LOCAL_INCLUDES += -I$(srcdir)/../../../../../media/libyuv/include
in objdir/media/webrtc/trunk/webrtc/common_video/common_video_common_video/backend.mk

OTOH, using 'libyuv_dir': '../../../../../media/libyuv' yields:
   LOCAL_INCLUDES += -I$(topsrcdir)/media/libyuv/include

which is correct too. I don't know how you got your paths. Please test /media/libyuv again.

::: media/webrtc/signaling/test/Makefile.in
@@ +12,5 @@
>    $(DEPTH)/media/webrtc/signalingtest/signaling_ecc/$(LIB_PREFIX)ecc.$(LIB_SUFFIX) \
>    $(DEPTH)/media/webrtc/signalingtest/signaling_sipcc/$(LIB_PREFIX)sipcc.$(LIB_SUFFIX) \
>    $(DEPTH)/layout/media/webrtc/$(LIB_PREFIX)webrtc.$(LIB_SUFFIX) \
>    $(DEPTH)/layout/media/$(LIB_PREFIX)gkmedias.$(LIB_SUFFIX) \
> +  $(DEPTH)/media/$(LIB_PREFIX)yuv/$(LIB_PREFIX)yuv_$(LIB_PREFIX)yuv/$(LIB_PREFIX)yuv.$(LIB_SUFFIX) \

I'd be very surprised if that works on windows, where LIB_PREFIX is nothing. Use media/libyuv/libyuv_libyuv.

Comment 19

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #18)
> Comment on attachment 8363669 [details] [diff] [review]
> Move libyuv to media/libyuv
> 
> Review of attachment 8363669 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Note I get this when applying:
> media/webrtc/trunk/third_party/libyuv/Android.mk not tracked!
> media/webrtc/trunk/third_party/libyuv/LICENSE_THIRD_PARTY not tracked!
> media/webrtc/trunk/third_party/libyuv/OWNERS not tracked!
> media/webrtc/trunk/third_party/libyuv/all.gyp no tracked!

Sorry, forgot to remove that part, which i figured was because i hadn't applied the libyuv update.

Comment 20

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

(In reply to Mike Hommey [:glandium] from comment #18)

> ::: build/gyp.mozbuild
> @@ +22,5 @@
> >      'use_system_libvpx': 0,
> >      'build_libjpeg': 0,
> >      'build_libvpx': 0,
> > +    'build_libyuv': 0,
> > +    'libyuv_dir': '../../../../../media/libyuv',
> 
> A local test shows this yields:
>    LOCAL_INCLUDES += -I$(srcdir)/../../../../../media/libyuv/include
> in
> objdir/media/webrtc/trunk/webrtc/common_video/common_video_common_video/
> backend.mk
> 
> OTOH, using 'libyuv_dir': '../../../../../media/libyuv' yields:
>    LOCAL_INCLUDES += -I$(topsrcdir)/media/libyuv/include
> 
> which is correct too. I don't know how you got your paths. Please test
> /media/libyuv again.

How are the libyuv_dir source values different?  You quote
'libyuv_dir': '../../../../../media/libyuv'
and my patch has:
+    'libyuv_dir': '../../../../../media/libyuv',

I think you meant to write: '/media/libyuv'

Interestingly, this now works.  Perhaps I was running an older rev of something when I made that test before, or fumbled which patches were applied.  So I'll switch it to /media/libyuv.  (Both work in any case, but this is better.)

> 
> ::: media/webrtc/signaling/test/Makefile.in
> @@ +12,5 @@
> >    $(DEPTH)/media/webrtc/signalingtest/signaling_ecc/$(LIB_PREFIX)ecc.$(LIB_SUFFIX) \
> >    $(DEPTH)/media/webrtc/signalingtest/signaling_sipcc/$(LIB_PREFIX)sipcc.$(LIB_SUFFIX) \
> >    $(DEPTH)/layout/media/webrtc/$(LIB_PREFIX)webrtc.$(LIB_SUFFIX) \
> >    $(DEPTH)/layout/media/$(LIB_PREFIX)gkmedias.$(LIB_SUFFIX) \
> > +  $(DEPTH)/media/$(LIB_PREFIX)yuv/$(LIB_PREFIX)yuv_$(LIB_PREFIX)yuv/$(LIB_PREFIX)yuv.$(LIB_SUFFIX) \
> 
> I'd be very surprised if that works on windows, where LIB_PREFIX is nothing.
> Use media/libyuv/libyuv_libyuv.

The signaling tests don't build on Windows currently... But I can make that change; no problem.

Comment 21

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

https://tbpl.mozilla.org/?tree=Try&rev=0e02065ce326
Green on Try

Comment 22

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

Attachment: Move libyuv to media/libyuv

Comment 23

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

Comment on attachment 8368187 [details] [diff] [review]
Move libyuv to media/libyuv

With the suggested changes

Comment 24

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

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

Comment 25

[Carsten Book [:Tomcat]]

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

Comment 26

[Carsten Book [:Tomcat]]

backed this out on central and integration branches as part of https://tbpl.mozilla.org/?rev=735a648bca0d because of the suspicion of causing bustages on device builds like https://tbpl.mozilla.org/php/getParsedLog.php?id=33874773&tree=Mozilla-Central

Comment 27

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

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

Comment 28

[Carsten Book [:Tomcat]]

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