Move webrtc/video_frame to common_video/include.

Move webrtc/video_frame to common_video/include.

BUG=webrtc:7616
Review-Url: https://codereview.webrtc.org/2877703002
Cr-Commit-Position: refs/heads/master@{#18147}
Committed: https://chromium.googlesource.com/external/webrtc/+/ea3a7987d247c2b63ada71b9e3ac076e2e10f387

[nisse-webrtc, 2017-05-11 07:13:34]

nisse@webrtc.org changed reviewers:
+ kwiberg@webrtc.org, mbonadei@webrtc.org

[nisse-webrtc, 2017-05-11 07:13:34]

This moves the header file, updates internal includes, and while at it, also
deleted the existing wrapper file common_video/include/video_image.h.

If we need a gn target for webrtc/video_frame.h (compatibility wrapper), what
should its name be?

https://codereview.webrtc.org/2877703002/diff/20001/webrtc/api/video_codecs/B...
File webrtc/api/video_codecs/BUILD.gn (right):

https://codereview.webrtc.org/2877703002/diff/20001/webrtc/api/video_codecs/B...
webrtc/api/video_codecs/BUILD.gn:27: "../../common_video:common_video",
Is this dependency appropriate? I'm not sure what's really in video_codecs_api.

[kwiberg-webrtc, 2017-05-12 09:06:24]

https://codereview.webrtc.org/2877703002/diff/20001/webrtc/api/video_codecs/B...
File webrtc/api/video_codecs/BUILD.gn (right):

https://codereview.webrtc.org/2877703002/diff/20001/webrtc/api/video_codecs/B...
webrtc/api/video_codecs/BUILD.gn:27: "../../common_video:common_video",
On 2017/05/11 07:13:34, nisse-webrtc wrote:
> Is this dependency appropriate? I'm not sure what's really in
video_codecs_api.

I think the answer is "whatever is declared in video_{en,de}coder.h". (By the
way, it's suspicious that there are no .cc files listed in sources. Do they
really not declare anything that they don't define?)

However, I *can* tell you that you can drop the ":common_video" suffix, since
it's implied if you omit it.

[nisse-webrtc, 2017-05-12 10:42:20]

https://codereview.webrtc.org/2877703002/diff/20001/webrtc/api/video_codecs/B...
File webrtc/api/video_codecs/BUILD.gn (right):

https://codereview.webrtc.org/2877703002/diff/20001/webrtc/api/video_codecs/B...
webrtc/api/video_codecs/BUILD.gn:27: "../../common_video:common_video",
On 2017/05/12 09:06:24, kwiberg-webrtc wrote:
> On 2017/05/11 07:13:34, nisse-webrtc wrote:
> > Is this dependency appropriate? I'm not sure what's really in
> video_codecs_api.
> 
> I think the answer is "whatever is declared in video_{en,de}coder.h". (By the
> way, it's suspicious that there are no .cc files listed in sources. Do they
> really not declare anything that they don't define?)

They declare interfaces only, it seems. Hence no source files. And the
interfaces depend on EncodedImage, so I guess this dependency is appropriate.

> However, I *can* tell you that you can drop the ":common_video" suffix, since
> it's implied if you omit it.

Deleted.

[mbonadei, 2017-05-12 12:01:49]

On 2017/05/12 10:42:20, nisse-webrtc wrote:
>
https://codereview.webrtc.org/2877703002/diff/20001/webrtc/api/video_codecs/B...
> File webrtc/api/video_codecs/BUILD.gn (right):
> 
>
https://codereview.webrtc.org/2877703002/diff/20001/webrtc/api/video_codecs/B...
> webrtc/api/video_codecs/BUILD.gn:27: "../../common_video:common_video",
> On 2017/05/12 09:06:24, kwiberg-webrtc wrote:
> > On 2017/05/11 07:13:34, nisse-webrtc wrote:
> > > Is this dependency appropriate? I'm not sure what's really in
> > video_codecs_api.
> > 
> > I think the answer is "whatever is declared in video_{en,de}coder.h". (By
the
> > way, it's suspicious that there are no .cc files listed in sources. Do they
> > really not declare anything that they don't define?)
> 
> They declare interfaces only, it seems. Hence no source files. And the
> interfaces depend on EncodedImage, so I guess this dependency is appropriate.
> 
> > However, I *can* tell you that you can drop the ":common_video" suffix,
since
> > it's implied if you omit it.
> 
> Deleted.

nisse@: thanks for this cleanup. I have no knowledge to comment about the 
video_codecs_api dependency on common_video. But lgtm for the BUILD.gn part.

[nisse-webrtc, 2017-05-12 12:30:03]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 12:30:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-12 13:05:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 13:05:11]

Dry run: This issue passed the CQ dry run.

[mbonadei, 2017-05-12 13:36:32]

https://codereview.webrtc.org/2877703002/diff/40001/webrtc/video_frame.h
File webrtc/video_frame.h (right):

https://codereview.webrtc.org/2877703002/diff/40001/webrtc/video_frame.h#newc...
webrtc/video_frame.h:14: // TODO(nisse): Delete this wrapper file, as soon as
downstream
I have just realized that there is no GN target for this header.
While we keep this for backward compatibility. Is is possible
to add a target (a source_set is fine) for this header in
webrtc/BUILD.gn?

[nisse-webrtc, 2017-05-12 15:07:44]

On 2017/05/12 13:36:32, mbonadei wrote:
> https://codereview.webrtc.org/2877703002/diff/40001/webrtc/video_frame.h
> File webrtc/video_frame.h (right):
> 
>
https://codereview.webrtc.org/2877703002/diff/40001/webrtc/video_frame.h#newc...
> webrtc/video_frame.h:14: // TODO(nisse): Delete this wrapper file, as soon as
> downstream
> I have just realized that there is no GN target for this header.
> While we keep this for backward compatibility. Is is possible
> to add a target (a source_set is fine) for this header in
> webrtc/BUILD.gn?

I could do that, if I get a suggestion for a name. But on the other
hand, is there any point in doing that?

Nothing in webrtc depends on that target. No downstream projects
depend on it either (since it doesn't yet exist). And any downstream
project which are updated, do we prefer that they keep using this
wrapper header file and then depend on the hypothetical new target? Or
are we better off if they use the common_video headers and targets?
I'd prefer to let them depend on common_video directly; we'd prefer to
not consider that a public api, but that's how the curent code works,
and indirection via other deprecated wrappers doesn't improve the
situation, imo.

[mbonadei, 2017-05-12 15:16:44]

On 2017/05/12 15:07:44, nisse-webrtc wrote:
> On 2017/05/12 13:36:32, mbonadei wrote:
> > https://codereview.webrtc.org/2877703002/diff/40001/webrtc/video_frame.h
> > File webrtc/video_frame.h (right):
> > 
> >
>
https://codereview.webrtc.org/2877703002/diff/40001/webrtc/video_frame.h#newc...
> > webrtc/video_frame.h:14: // TODO(nisse): Delete this wrapper file, as soon
as
> > downstream
> > I have just realized that there is no GN target for this header.
> > While we keep this for backward compatibility. Is is possible
> > to add a target (a source_set is fine) for this header in
> > webrtc/BUILD.gn?
> 
> I could do that, if I get a suggestion for a name. But on the other
> hand, is there any point in doing that?
> 
> Nothing in webrtc depends on that target. No downstream projects
> depend on it either (since it doesn't yet exist). And any downstream
> project which are updated, do we prefer that they keep using this
> wrapper header file and then depend on the hypothetical new target? Or
> are we better off if they use the common_video headers and targets?
> I'd prefer to let them depend on common_video directly; we'd prefer to
> not consider that a public api, but that's how the curent code works,
> and indirection via other deprecated wrappers doesn't improve the
> situation, imo.

Yep, it is better if they start to depend on common_video directly.
The GN target is basically used by our build to ensure that no one will
#include the target without declaring it to GN.
Since I don't know how long it will stay around and we are also trying to
track all the headers with GN to avoid/reduce new cyclic dependencies I
was thinking to create a source_set just for the sake of tracking it.

I always struggle with naming. :)
I would also go with something like "video_frame_deprecated" to make it
explicit.

[nisse-webrtc, 2017-05-15 06:25:31]

On 2017/05/12 15:16:44, mbonadei wrote:
> Yep, it is better if they start to depend on common_video directly.
> The GN target is basically used by our build to ensure that no one will
> #include the target without declaring it to GN.

No webrtc code should include the header anymore, with or without gn target...

> I would also go with something like "video_frame_deprecated" to make it
> explicit.

Done.

[mbonadei, 2017-05-15 06:58:48]

On 2017/05/15 06:25:31, nisse-webrtc wrote:
> On 2017/05/12 15:16:44, mbonadei wrote:
> > Yep, it is better if they start to depend on common_video directly.
> > The GN target is basically used by our build to ensure that no one will
> > #include the target without declaring it to GN.
> 
> No webrtc code should include the header anymore, with or without gn target...
> 
> > I would also go with something like "video_frame_deprecated" to make it
> > explicit.
> 
> Done.

lgtm

[nisse-webrtc, 2017-05-15 07:06:14]

On 2017/05/15 06:58:48, mbonadei wrote:
> On 2017/05/15 06:25:31, nisse-webrtc wrote:
> > On 2017/05/12 15:16:44, mbonadei wrote:
> > > Yep, it is better if they start to depend on common_video directly.
> > > The GN target is basically used by our build to ensure that no one will
> > > #include the target without declaring it to GN.
> > 
> > No webrtc code should include the header anymore, with or without gn
target...
> > 
> > > I would also go with something like "video_frame_deprecated" to make it
> > > explicit.
> > 
> > Done.
> 
> lgtm

Thanks. Karl, any further comments?

[kwiberg-webrtc, 2017-05-15 07:27:04]

https://codereview.webrtc.org/2877703002/diff/60001/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/2877703002/diff/60001/webrtc/BUILD.gn#newcode243
webrtc/BUILD.gn:243: }
Hmm. This source_set should have a dep on the target the moved .h file belongs
to, right? I would've thought gn check would flag that...

[mbonadei, 2017-05-15 07:31:56]

https://codereview.webrtc.org/2877703002/diff/60001/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/2877703002/diff/60001/webrtc/BUILD.gn#newcode243
webrtc/BUILD.gn:243: }
On 2017/05/15 07:27:04, kwiberg-webrtc wrote:
> Hmm. This source_set should have a dep on the target the moved .h file belongs
> to, right? I would've thought gn check would flag that...

Yes, you are right.

Gn is not catching this because in the .gn file we are not checking the webrtc
directory [1].
We will do it soon. We have to land [2] and then we will enable it on
the whole codebase.

This is a good catch. Thanks!

[1] - https://chromium.googlesource.com/external/webrtc/+/master/.gn#24
[2] - https://codereview.webrtc.org/2845323002/

[nisse-webrtc, 2017-05-15 08:32:55]

https://codereview.webrtc.org/2877703002/diff/60001/webrtc/BUILD.gn
File webrtc/BUILD.gn (right):

https://codereview.webrtc.org/2877703002/diff/60001/webrtc/BUILD.gn#newcode243
webrtc/BUILD.gn:243: }
On 2017/05/15 07:27:04, kwiberg-webrtc wrote:
> Hmm. This source_set should have a dep on the target the moved .h file belongs
> to, right? I would've thought gn check would flag that...

Added dependency.

[kwiberg-webrtc, 2017-05-15 08:45:56]

lgtm

[nisse-webrtc, 2017-05-15 08:46:00]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 08:46:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[nisse-webrtc, 2017-05-15 08:46:10]

The CQ bit was unchecked by nisse@webrtc.org

[nisse-webrtc, 2017-05-15 08:46:10]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-05-15 08:46:11]

The patchset sent to the CQ was uploaded after l-g-t-m from mbonadei@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2877703002/#ps80001 (title:
"Add gn dependency.")

[commit-bot: I haz the power, 2017-05-15 08:46:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-15 09:04:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 09:04:22]

Try jobs failed on following builders:
  win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...)

[nisse-webrtc, 2017-05-15 09:34:36]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2017-05-15 09:34:47]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-15 09:42:12]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494840876099480,
"parent_rev": "c99bddfc1c38c67f709d821bc82ba21a9c889165", "commit_rev":
"ea3a7987d247c2b63ada71b9e3ac076e2e10f387"}

[commit-bot: I haz the power, 2017-05-15 09:42:17]

Description was changed from

==========
Move webrtc/video_frame to common_video/include.

BUG=webrtc:7616
==========

to

==========
Move webrtc/video_frame to common_video/include.

BUG=webrtc:7616

Review-Url: https://codereview.webrtc.org/2877703002
Cr-Commit-Position: refs/heads/master@{#18147}
Committed:
https://chromium.googlesource.com/external/webrtc/+/ea3a7987d247c2b63ada71b9e...
==========

[commit-bot: I haz the power, 2017-05-15 09:42:18]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/ea3a7987d247c2b63ada71b9e...