Use relative filenames for internal includes.

Use relative filenames for internal includes.

This makes it possible for applications to use, e.g., 

  #include "somewhere/libyuv/include/libyuv/convert.h"

without arranging so that libyuv's include directory is added with -I on the compiler command line.

BUG=

[nisse-chromium (ooo August 14), 2016-05-13 11:05:14]

Description was changed from

==========
Use relative filenames for internal includes.

BUG=
==========

to

==========
Use relative filenames for internal includes.

This makes it possible for applications to use, e.g., 

  #include "somewhere/libyuv/include/libyuv/convert.h"

without arranging so that libyuv's include directory is added with -I on the
compiler command line.

BUG=
==========

[nisse-chromium (ooo August 14), 2016-05-13 11:06:44]

nisse@chromium.org changed reviewers:
+ magjed@chromium.org

[magjed_chromium, 2016-05-13 11:41:02]

magjed@chromium.org changed reviewers:
+ kjellander@chromium.org

[magjed_chromium, 2016-05-13 11:43:54]

+kjellander

kjellander, can you help verify that changing all:
#include "libyuv/xxx.h"
to
#include "xxx.h"
in libyuv is safe?

[kjellander_chromium, 2016-05-13 12:22:11]

kjellander@chromium.org changed reviewers:
+ fbarchard@chromium.org

[kjellander_chromium, 2016-05-13 12:23:36]

On 2016/05/13 11:43:54, magjed_chromium wrote:
> +kjellander
> 
> kjellander, can you help verify that changing all:
> #include "libyuv/xxx.h"
> to
> #include "xxx.h"
> in libyuv is safe?

No, I don't have that good insight in all the downstream customers using libyuv.
I though one wanted to have directories (or even absolute paths like in
WebRTC/Chromium) if possible?

I added fbarchard@ regarding what effects this change could have.

[nisse-chromium (ooo August 14), 2016-05-13 12:59:52]

On 2016/05/13 12:23:36, kjellander (chromium) wrote:
> On 2016/05/13 11:43:54, magjed_chromium wrote:
> > +kjellander
> > 
> > kjellander, can you help verify that changing all:
> > #include "libyuv/xxx.h"
> > to
> > #include "xxx.h"
> > in libyuv is safe?
> 
> No, I don't have that good insight in all the downstream customers using
libyuv.
> I though one wanted to have directories (or even absolute paths like in
> WebRTC/Chromium) if possible?
> 
> I added fbarchard@ regarding what effects this change could have.

My understanding is that #include "foo.h" in a C file will always get the foo.h
file in the same directory, if it exists. Which seems like the right thing to do
when, e.g., libyuv's convert.h includes libyuv's own rotate.h.

For an application, the only (working) scenario where this would be a change is
if there are two different libyuv trees, say in dir-A and dir-B, the application
adds -I "dir-A/libyuv/include" to the compilation command line, and then
compiles some C file doing 

  #include "dir-B/libyuv/include/libyuv/convert.h".

Before this change, the include directive in in dir-B/.../convert.h will resolve
to dir-A/.../rotate.h. After this change, the include will resolve to
dir-B/.../rotate.h, i.e., the file in the same directory.

And the case I want to solve is as above, but without the -I flag; then before
this change, rotate.h isn't found at all and compilation fails.

[nisse-chromium (ooo August 14), 2016-05-17 09:07:32]

For a bit more context, webrtc's common_video.gyp includes the conditional

        ['build_libyuv==1', {
          'dependencies': ['<(DEPTH)/third_party/libyuv/libyuv.gyp:libyuv',],
          'export_dependent_settings': [
            '<(DEPTH)/third_party/libyuv/libyuv.gyp:libyuv',
          ],
        }, {
          # Need to add a directory normally exported by libyuv.gyp.
          'include_dirs': ['<(libyuv_dir)/include',],
        }],

I'm not sure how this is supposed to work (use other libyuv under third_party,
or a library installed on the system?). With this cl, it's no longer necessary
to add anything to include_dirs, webrtc files should be able to

#include "third_party/libyuv/include/libyuv/scale.h"

Which in turn makes it possible to use libyuv functions directly from any places
that need them, even outside of common_video, and eliminate the wrapper
functions in webrtc/common_video/libyuv/webrtc_libyuv.cc.

[magjed_chromium, 2016-05-18 10:29:30]

On 2016/05/17 09:07:32, nisse-chromium wrote:
> For a bit more context, webrtc's common_video.gyp includes the conditional
> 
>         ['build_libyuv==1', {
>           'dependencies': ['<(DEPTH)/third_party/libyuv/libyuv.gyp:libyuv',],
>           'export_dependent_settings': [
>             '<(DEPTH)/third_party/libyuv/libyuv.gyp:libyuv',
>           ],
>         }, {
>           # Need to add a directory normally exported by libyuv.gyp.
>           'include_dirs': ['<(libyuv_dir)/include',],
>         }],
> 
> I'm not sure how this is supposed to work (use other libyuv under third_party,
> or a library installed on the system?). With this cl, it's no longer necessary
> to add anything to include_dirs, webrtc files should be able to
> 
> #include "third_party/libyuv/include/libyuv/scale.h"
> 
> Which in turn makes it possible to use libyuv functions directly from any
places
> that need them, even outside of common_video, and eliminate the wrapper
> functions in webrtc/common_video/libyuv/webrtc_libyuv.cc.

Make sure this follows the Google style guide
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes.

[nisse-chromium (ooo August 14), 2016-05-18 12:04:15]

On 2016/05/18 10:29:30, magjed_chromium wrote:
> Make sure this follows the Google style guide
> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes.

I'm afraid it doesn't.

If libyuv keeps using non-relative names for internal includes, then webrtc (and
any other projects depending on libyuv) ought to stop using 

  #include "third_party/.../include/libyuv/scaler.h"

since that basically isn't a supported way to get access to libyuv headers. Is
that the intention of the style guide? 

I mainly want to have some consistency; I see that banning use of
third_party/... like above also has advantages, e.g., it makes it a lot easier
to configure the webrtc build to use libyuv headers from /usr/include.

[kjellander_chromium, 2016-05-23 07:17:56]

On 2016/05/18 12:04:15, nisse-chromium wrote:
> On 2016/05/18 10:29:30, magjed_chromium wrote:
> > Make sure this follows the Google style guide
> >
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes.
> 
> I'm afraid it doesn't.
> 
> If libyuv keeps using non-relative names for internal includes, then webrtc
(and
> any other projects depending on libyuv) ought to stop using 
> 
>   #include "third_party/.../include/libyuv/scaler.h"
> 
> since that basically isn't a supported way to get access to libyuv headers. Is
> that the intention of the style guide? 
> 
> I mainly want to have some consistency; I see that banning use of
> third_party/... like above also has advantages, e.g., it makes it a lot easier
> to configure the webrtc build to use libyuv headers from /usr/include.

I'm not an expert, but I think it may make sense to use shallow include paths
like suggested in this CL for a leaf-dependency project like libyuv. Here are
some other examples that do so:
https://chromium.googlesource.com/chromium/deps/libjpeg_turbo/+/master
https://chromium.googlesource.com/chromium/deps/libsrtp/+/master/srtp/crypto/...

libvpx does so for some, but due to the size of that project, it has some
directory hierarchy, I assume mostly for organizational reasons. E.g.
https://chromium.googlesource.com/chromium/deps/libvpx/+/master/source/libvpx...

There's also some examples of includes that uses include paths like #include
"./foo.h", what's does that change?

I still would like to hear Frank's opinion on why it's setup with the libyuv
subdirectory from the beginning, since I assume such a change may break other
downstream projects we're not aware of.

[nisse-chromium (ooo August 14), 2016-05-23 08:47:57]

On 2016/05/23 07:17:56, kjellander (chromium) wrote:

> I'm not an expert, but I think it may make sense to use shallow include paths
> like suggested in this CL for a leaf-dependency project like libyuv. Here are
> some other examples that do so:
> https://chromium.googlesource.com/chromium/deps/libjpeg_turbo/+/master
>
https://chromium.googlesource.com/chromium/deps/libsrtp/+/master/srtp/crypto/...

Maybe one could also interpret the style guide as primarily saying how to write
the #include lines in C and C++ source files, and be a bit more relaxed about
header files including each other.
 
> There's also some examples of includes that uses include paths like #include
> "./foo.h", what's does that change?

Here "." refers to the directory where the compiler was invoked (i.e, the
compiler process' current directory), which may be different from the location
of the source file being compiled. Not sure if it's kosher from a C standard
perspective.

E.g, if you have a read-only source tree and a separate build tree, and generate
foo.h at compile time into the build directory. Then #include "foo.h" won't find
it, but #include "./foo.h" will. Packages using autoconf usually add -I. to the
compilation flags to be able to pick up files in the build tree, without adding
any ./ prefixes to the #include lines.

[nisse-chromium (ooo August 14), 2016-08-29 06:43:33]

Just let me give one more concrete example. If you don't agree, I'll just have
to drop this.

I want to call libyuv directly from
webrtc/examples/peerconnection/client/linux/main_wnd.cc, in order to delete the
method cricket::VideoFrame::ConvertToRgbBuffer.

I believe the proper way to include libyuv headers in webrtc is to use path from
the webrtc root, like

#include "third_party/libyuv/include/libyuv/convert_from.h"

and that's how other third_party headers are handled. Is that wrong? However,
that fails with the error

  ../../third_party/libyuv/include/libyuv/convert_from.h:14:10: fatal error:
'libyuv/basic_types.h' file not found
  #include "libyuv/basic_types.h"
         ^

So the current way internal libyuv includes are done forces webrtc to add
libyuv-specific -I flags on the compiler command line.

To sum up pros and cons of this cl:

+ Makes include paths from webrtc root (and similarly for other projects
depending on libyuv) always work.

- In case we want to support building webrtc using libyuv installed under the
system's /usr/include, or some other global location, instead of the copy under
third_party/, they we can't use include paths like at webrtc/.../libyuv anyway,
and then this change is pointless (but still harmless).

[fbarchard1, 2016-08-29 17:59:44]

Re I believe the proper way to include libyuv headers in webrtc is to use path
from
the webrtc root, like #include
"third_party/libyuv/include/libyuv/convert_from.h"

My understanding is if you depend on libyuv in gyp:
'../third_party/libyuv/libyuv.gyp:libyuv',
and make direct calls to libyuv, you should add the directory to your includes
          'include_dirs': [
            '<(DEPTH)/third_party/libyuv',
and use #include "libyuv/convert_from.h"
          ],
You should not include third_party etc in your include, as that wont work in
different tree structures.

A relative name (#include "convert_from.h") is ambiguous and may find the wrong
header?  thats why we moved away from that early on?

I did do work on this bug to reduce internal, unnecessary includes.  It caused
build issues for webrtc and chromoting that depended on the includes, so I made
changes to both webrtc and chromoting, but had to leave in some extra includes. 
Would be good to have a libyuv issue submitted.

[nisse-chromium (ooo August 14), 2016-08-30 06:26:22]

On 2016/08/29 17:59:44, fbarchard1 wrote:

> A relative name (#include "convert_from.h") is ambiguous and may find the
wrong
> header?

No, in the case at ahnd, where the file "convert_from.h" is located in the same
directory as the file containing the #include "convert_from.h" directive,
there's no ambiguity.

> thats why we moved away from that early on?

Don't know, I'm afraid I'm not aware of that history.
 
> I did do work on this bug to reduce internal, unnecessary includes.

Nice. But I guess libyuv/basic_types.h is included by most libyuv headers?

To get the needed -I flags on the compiler command line for webrtc, it seems
sufficient to add a direct libyuv dependency to the target in question. Like in

https://codereview.webrtc.org/2287233002/diff/20001/webrtc/examples/BUILD.gn

I'll close this if I don't get more support for the change in the next few days.
Thanks for your review!