Fix missing symbols for pre-linking of the Cast sender library for iOS.

Fix missing symbols for pre-linking of the Cast sender library for iOS.

When pre-linking the Cast sender library for the iOS simulator, a number
of symbols are reported as undefined, even though the functions using
them are dead code and the whole chain should disappear under dead code
elimination. The same problem does not occur when targetting iOS
hardware, which suggests a bug in the Intel linker.

This patch either defines those missing symbols or prevents their use.

Specifically, the patch adds dummy definitions for native_library and
process_metrics under iOS which do nothing, and disables the assembly
code path in yuv_convert when compiling for iOS (gyp is unable to build
the simd code properly for target_subarch=both). Otherwise yuv_convert
is functional via its normal CPU code path and is added to the
media_for_ios_cast target to support VideoFrame methods.

BUG=470602
R=miu@chromium.org

Committed: https://crrev.com/32be1d41c5fff2548c3dbe7406cc082a78b3a004
Cr-Commit-Position: refs/heads/master@{#323267}

[jfroy, 2015-03-25 18:31:00]

jfroy@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[jfroy, 2015-03-25 18:31:37]

jfroy@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-03-25 18:35:39]

https://codereview.chromium.org/1030253002/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/1030253002/diff/1/media/base/video_frame.cc#n...
media/base/video_frame.cc:483: scoped_refptr<VideoFrame>
VideoFrame::CreateColorFrame(
So you use this class in cast, but you rely on nothing calling
CreateColorFrame()? That seems somewhat brittle…

[jfroy, 2015-03-25 18:40:21]

On 2015/03/25 18:35:39, Nico wrote:
> https://codereview.chromium.org/1030253002/diff/1/media/base/video_frame.cc
> File media/base/video_frame.cc (right):
> 
>
https://codereview.chromium.org/1030253002/diff/1/media/base/video_frame.cc#n...
> media/base/video_frame.cc:483: scoped_refptr<VideoFrame>
> VideoFrame::CreateColorFrame(
> So you use this class in cast, but you rely on nothing calling
> CreateColorFrame()? That seems somewhat brittle…

My understanding is that those functions are primarily used for tests.

I don't think there is any brittleness, in so far as if a required symbol is
missing, it won't link. I expect / hope that GN will resolve many of the iOS
build issues related to assembly code, which has been a pain to deal with in the
media component and is the primary reason for the existence of the "media for
cast ios" target. Under normal builds, media is not built at all for iOS.

[miu, 2015-03-27 21:26:26]

https://codereview.chromium.org/1030253002/diff/20001/base/native_library_ios.mm
File base/native_library_ios.mm (right):

https://codereview.chromium.org/1030253002/diff/20001/base/native_library_ios...
base/native_library_ios.mm:21: delete library;
Rather than delete library, consider:

  DCHECK(!library);

Since there shouldn't be a library to delete on IOS.

https://codereview.chromium.org/1030253002/diff/20001/base/native_library_ios...
base/native_library_ios.mm:27: return nullptr;
Consider adding NOTREACHED()

https://codereview.chromium.org/1030253002/diff/20001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/1030253002/diff/20001/media/base/video_frame....
media/base/video_frame.cc:483: scoped_refptr<VideoFrame>
VideoFrame::CreateColorFrame(
I don't understand: Why can't these be compiled in for IOS always?  I don't see
any special "depends on libyuv" or "calls into custom asm code" here.  FillYUV()
just memset's everything.

If I'm wrong about this, consider leaving the functions defined, and just wrap
the calls to FillYUV() with the #if !defined(...), and please be sure to comment
why.

[jfroy, 2015-03-27 21:33:26]

https://codereview.chromium.org/1030253002/diff/20001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/1030253002/diff/20001/media/base/video_frame....
media/base/video_frame.cc:483: scoped_refptr<VideoFrame>
VideoFrame::CreateColorFrame(
On 2015/03/27 21:26:26, miu wrote:
> I don't understand: Why can't these be compiled in for IOS always?  I don't
see
> any special "depends on libyuv" or "calls into custom asm code" here. 
FillYUV()
> just memset's everything.
> 
> If I'm wrong about this, consider leaving the functions defined, and just wrap
> the calls to FillYUV() with the #if !defined(...), and please be sure to
comment
> why.

FillYUV is defined in yuv_convert.cc, which will select CPU-optimized
implementations. The problem is that it selects SSE functions when compiling for
the iOS simulator, but the simd code is not properly compiled when configuring
the build with target_subarch=both target_arch=arm. If you try to enable the
simd code for the simulator, you end up with a broken iOS hardware build.

It is really better to leave the functions defined when they won't in fact work
correctly? Seems worse than defining them out.

Ultimately, and from what I can tell from the bug tracker, those build issues
will be fixed in GN.

[miu, 2015-03-27 22:05:07]

On 2015/03/27 21:33:26, jfroy wrote:
> FillYUV is defined in yuv_convert.cc, which will select CPU-optimized
> implementations. The problem is that it selects SSE functions when compiling
for
> the iOS simulator, but the simd code is not properly compiled when configuring
> the build with target_subarch=both target_arch=arm. If you try to enable the
> simd code for the simulator, you end up with a broken iOS hardware build.

From code search, it looks like media::FillYUV() is being called and not
libyuv::FillYUV():

https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_u...

[jfroy, 2015-03-27 22:21:36]

On 2015/03/27 22:05:07, miu wrote:
> On 2015/03/27 21:33:26, jfroy wrote:
> > FillYUV is defined in yuv_convert.cc, which will select CPU-optimized
> > implementations. The problem is that it selects SSE functions when compiling
> for
> > the iOS simulator, but the simd code is not properly compiled when
configuring
> > the build with target_subarch=both target_arch=arm. If you try to enable the
> > simd code for the simulator, you end up with a broken iOS hardware build.
> 
> From code search, it looks like media::FillYUV() is being called and not
> libyuv::FillYUV():
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_u...

That's right, and media::FillYUV in turn relies on media/base/simd (for i386 and
x86_64).

[jfroy, 2015-03-30 18:12:50]

https://codereview.chromium.org/1030253002/diff/20001/base/native_library_ios.mm
File base/native_library_ios.mm (right):

https://codereview.chromium.org/1030253002/diff/20001/base/native_library_ios...
base/native_library_ios.mm:21: delete library;
On 2015/03/27 21:26:26, miu wrote:
> Rather than delete library, consider:
> 
>   DCHECK(!library);
> 
> Since there shouldn't be a library to delete on IOS.

Done.

https://codereview.chromium.org/1030253002/diff/20001/base/native_library_ios...
base/native_library_ios.mm:27: return nullptr;
On 2015/03/27 21:26:26, miu wrote:
> Consider adding NOTREACHED()

Done.

[miu, 2015-03-31 01:42:48]

On 2015/03/27 22:21:36, jfroy wrote:
> On 2015/03/27 22:05:07, miu wrote:
> > On 2015/03/27 21:33:26, jfroy wrote:
> > From code search, it looks like media::FillYUV() is being called and not
> > libyuv::FillYUV():
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_u...
> 
> That's right, and media::FillYUV in turn relies on media/base/simd (for i386
and
> x86_64).

Wait.  Are we looking at the same code?  ;-)  I'm looking at media::FillYUV in
src/media/base/video_util.cc and it only calls accessor methods on VideoFrame,
and memset().  I don't see how it uses code in media/base/simd.  Unless, is
there a linkage problem where some other function in video_util.cc requires
media/base/simd as a dependency?

[miu, 2015-03-31 01:44:21]

lgtm, assuming the module containing FillYUV() is creating linkage problems even
though there's no direct code path to media/base/simd for IOS.

[jfroy, 2015-03-31 01:49:39]

On 2015/03/31 01:42:48, miu wrote:
> On 2015/03/27 22:21:36, jfroy wrote:
> > On 2015/03/27 22:05:07, miu wrote:
> > > On 2015/03/27 21:33:26, jfroy wrote:
> > > From code search, it looks like media::FillYUV() is being called and not
> > > libyuv::FillYUV():
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/video_u...
> > 
> > That's right, and media::FillYUV in turn relies on media/base/simd (for i386
> and
> > x86_64).
> 
> Wait.  Are we looking at the same code?  ;-)  I'm looking at media::FillYUV in
> src/media/base/video_util.cc and it only calls accessor methods on VideoFrame,
> and memset().  I don't see how it uses code in media/base/simd.  Unless, is
> there a linkage problem where some other function in video_util.cc requires
> media/base/simd as a dependency?

Doh I'm sorry, you are right. Brain malfunction. The issue is not FillYUV
itself, but the call to ConvertRGB32ToYUV in CopyRGBToVideoFrame.
ConvertRGB32ToYUV() is provided by the yuv_convert TU and that in turn depends
on the simd code. So I could also stub out CopyRGBToVideoFrame() from iOS media
for cast builds and leave VideoFrame's API intact.

[miu, 2015-03-31 01:56:39]

On 2015/03/31 01:49:39, jfroy wrote:
> The issue is not FillYUV
> itself, but the call to ConvertRGB32ToYUV in CopyRGBToVideoFrame.
> ConvertRGB32ToYUV() is provided by the yuv_convert TU and that in turn depends
> on the simd code. So I could also stub out CopyRGBToVideoFrame() from iOS
media
> for cast builds and leave VideoFrame's API intact.

sgtm!  :)  Generally, I prefer to keep APIs intact where possible, and
stubbing-out where platform-specific implementations diverge.

[Nico, 2015-03-31 16:26:00]

lgtm

nit: Should the stubbed out base functions have NOTIMPLEMENTED()s, or "// Not
implemented." comments?

[DaleCurtis, 2015-03-31 18:17:16]

lgtm

[miu, 2015-03-31 19:35:23]

still lgtm

[miu, 2015-03-31 19:37:13]

On 2015/03/31 19:35:23, miu wrote:
> still lgtm

Whoops!  Wrong review.  But, assuming you plan to stub-out at the deeper level
as discussed, lgtm.

[jfroy, 2015-03-31 19:40:38]

I wrote another revision that changes yuv_convert to disable the assembly
functions for OS_IOS. I can then compile the C code path for both hardware
and simulator and lose no functionality. I just finished testing it. Will
upload soon.

@thakis: I'll add NOTIMPLEMENTED annotations.
On Tue, Mar 31, 2015 at 12:35 <miu@chromium.org> wrote:

> still lgtm
>
> https://codereview.chromium.org/1030253002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jfroy, 2015-03-31 21:01:34]

Since I changed a few things since the last round of lgtm, I'm going to hold off
on submitting to the CQ until OWNERS have had a chance to check the code again.
Not going to ninja unreviewed code in.

[Nico, 2015-03-31 21:45:58]

base still lgtm

[miu, 2015-03-31 23:34:04]

PS6 lgtm.

[DaleCurtis, 2015-04-01 16:03:02]

lgtm

[jfroy, 2015-04-01 17:05:37]

The CQ bit was checked by jfroy@chromium.org

[jfroy, 2015-04-01 17:05:37]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1030253002/#ps120001
(title: "Update media/base/BUILD.gn.")

[commit-bot: I haz the power, 2015-04-01 17:06:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1030253002/120001

[commit-bot: I haz the power, 2015-04-01 17:10:41]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-04-01 17:11:54]

Patchset 7 (id:??) landed as
https://crrev.com/32be1d41c5fff2548c3dbe7406cc082a78b3a004
Cr-Commit-Position: refs/heads/master@{#323267}