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

RESOLVED FIXED in mozilla30

Status

()

defect
P3
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: padenot, Assigned: jesup)

Tracking

unspecified
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC] [blocking-webrtc-])

Attachments

(3 attachments, 3 obsolete attachments)

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.
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.
Priority: -- → P3
Whiteboard: [WebRTC] [blocking-webrtc-]
Attachment #779599 - Flags: review?(rjesup)
Attachment #779600 - Flags: review?(rjesup)
Assignee: nobody → m_kato
also, https://code.google.com/p/libyuv/issues/detail?id=252 for upstream to export MJpegDecoder that uses WebRTC.
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.
Attachment #779600 - Flags: review?(rjesup) → review+
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.
(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.
Attachment #779599 - Attachment is obsolete: true
Attachment #779599 - Flags: review?(rjesup)
Attachment #780210 - Flags: review?(rjesup)
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
Attachment #780210 - Flags: review?(rjesup) → review+
Posted patch Move libyuv to media/libyuv (obsolete) — Splinter Review
builds, now depends on bug 880419
Depends on: 880419
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
Attachment #8361311 - Flags: review?(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.
Attachment #8361311 - Flags: review?(gps) → review?(mh+mozilla)
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
Attachment #8361311 - Flags: review?(mh+mozilla) → feedback-
(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 on attachment 8363669 [details] [diff] [review]
Move libyuv to media/libyuv

As mentioned, depends on the update to libyuv
Attachment #8363669 - Flags: review?(mh+mozilla)
Attachment #8361311 - Attachment is obsolete: true
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.
Attachment #8363669 - Flags: review?(mh+mozilla) → feedback+
(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.
(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.
Attachment #8363669 - Attachment is obsolete: true
Comment on attachment 8368187 [details] [diff] [review]
Move libyuv to media/libyuv

With the suggested changes
Attachment #8368187 - Flags: review?(mh+mozilla)
Attachment #8368187 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/be17fb052374
Assignee: m_kato → rjesup
Target Milestone: --- → mozilla29
https://hg.mozilla.org/mozilla-central/rev/be17fb052374
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/f9315e1a3844
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.