content/common/gpu/media add platform suffix to some files (cleanup)

content/common/gpu/media add platform suffix to some files (cleanup)

to make more evident which file is in which platform,
plus they get automatically removed from build lists
in the appropriate way.

BUG=

TBR=avi@chromium.org for the changes not covered by
sandersd@ and posciak@ :
- content/common/BUILD.gn
- content/common/sandbox_mac.mm
- content/gpugpu_main.cc
 (those are just renaming of the appropriate files).

Committed: https://crrev.com/7479738f45a79b6262a08be4e81e901e519fcb30
Cr-Commit-Position: refs/heads/master@{#363061}

[mcasas, 2015-11-24 21:10:51]

Patchset #1 (id:1) has been deleted

[mcasas, 2015-11-24 22:58:28]

mcasas@chromium.org changed reviewers:
+ dalecurtis@chromium.org, posciak@chromium.org

[mcasas, 2015-11-24 22:58:29]

posciak@/dalecurtis@ Owners RS/ PTAL

0 added code.

[mcasas, 2015-11-24 22:59:09]

Description was changed from

==========
[WIP] content/common/gpu/media add platform suffix to some files

BUG=552520
==========

to

==========
content/common/gpu/media add platform suffix to some files

to make more evident which file is in which platform,
plus they get automatically removed from build lists
in the appropriate way.

BUG=552520
==========

[DaleCurtis, 2015-11-24 23:43:00]

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

[DaleCurtis, 2015-11-24 23:43:00]

seems fine, but => sandersd@ since he's the owner for this stuff now.

[DaleCurtis, 2015-11-24 23:43:21]

dalecurtis@chromium.org changed reviewers:
- dalecurtis@chromium.org

[sandersd (OOO until July 31), 2015-11-25 00:42:51]

LGTM % fix CL title.

[Pawel Osciak, 2015-11-25 01:01:40]

I like the change apart from renaming the vea unittest. It should be usable on
other platforms as well...

[Pawel Osciak, 2015-11-25 01:03:43]

On 2015/11/25 01:01:40, Pawel Osciak wrote:
> I like the change apart from renaming the vea unittest. It should be usable on
> other platforms as well...

Specifically, FakeVideoEncodeAccelerator is usable on any platform, and VaapiVEA
should be usable on any Linux.

[mcasas, 2015-11-25 01:31:08]

Description was changed from

==========
content/common/gpu/media add platform suffix to some files

to make more evident which file is in which platform,
plus they get automatically removed from build lists
in the appropriate way.

BUG=552520
==========

to

==========
content/common/gpu/media add platform suffix to some files (cleanup)

to make more evident which file is in which platform,
plus they get automatically removed from build lists
in the appropriate way.

BUG=552520
==========

[mcasas, 2015-11-25 17:34:16]

On 2015/11/25 01:03:43, Pawel Osciak wrote:
> On 2015/11/25 01:01:40, Pawel Osciak wrote:
> > I like the change apart from renaming the vea unittest. It should be usable
on
> > other platforms as well...
> 
> Specifically, FakeVideoEncodeAccelerator is usable on any platform, and
VaapiVEA
> should be usable on any Linux.

Chromium is based on the law of demeter, or the minimal
needed knowledge: e.g. class definitions are limited to what 
is used on ToT, a new pixel format is only introduced if it's 
used, etc. In that line of reasoning, the fact that an api, or
a file, or class, is potentially usable in several platforms is 
irrelevant versus the use in Chromium as of ToT. Think about
a developer/tester coming to the codebase, wouldn't they 
appreciate the suffixes indicating where the file is _used_?
And remember code is read many more times than written,
and most likely by persons less confident/experienced/
knowledgeable of the codebase and the hardware APIs.

So:
- VEA unittest is only compiled in chromeos, so, to reflect
that situation, it should have its suffix. If and when the file
and its contents find its application to other platforms, which
I encourage, then the file can be, by all means, renamed. 
- FakeVEA, I would love to says is _chromeos specific, but
is also used from media/cast/sender/fake_video_encode_accelerator_factory.cc
- VAAPI _might_, in the future, be used for a number of
platforms beyond Windows, but as of ToT, it isn't. So,
please add the suffix to reflect this situation.

Makes sense?

[Pawel Osciak, 2015-11-26 01:56:51]

On 2015/11/25 17:34:16, mcasas wrote:
> On 2015/11/25 01:03:43, Pawel Osciak wrote:
> > On 2015/11/25 01:01:40, Pawel Osciak wrote:
> > > I like the change apart from renaming the vea unittest. It should be
usable
> on
> > > other platforms as well...
> > 
> > Specifically, FakeVideoEncodeAccelerator is usable on any platform, and
> VaapiVEA
> > should be usable on any Linux.
> 
> Chromium is based on the law of demeter, or the minimal
> needed knowledge: e.g. class definitions are limited to what 
> is used on ToT, a new pixel format is only introduced if it's 
> used, etc.
> In that line of reasoning, the fact that an api, or
> a file, or class, is potentially usable in several platforms is 
> irrelevant versus the use in Chromium as of ToT. Think about
> a developer/tester coming to the codebase, wouldn't they 
> appreciate the suffixes indicating where the file is _used_?


I'm sorry, my previous reply was not reflecting the issue at all. Please allow
me to retract that and correct myself.

It should actually not be about VAAPI or V4L2 API being potentially usable on
other platforms, it's about VEA API being cross-platform and this being a test
for VEA API, not the platform-specific implementations.

VEA unittest is a test for the platform-independent VEA API. The test *only*
calls VEA API, not VAAPI, not V4L2, etc., and has no specifics of these two
platform-specific APIs. Anyone can come, and, for example, add a
VEAClient::CreateAndroidVEA() method and use it on Android VEA, and the test
will work with it. VEA is a cross-platform API used not only on ChromeOS. I'm
actually surprised we don't have this CreateAndroidVEA(), looks like an
omission.

There are two reasons why it is compiled only on ChromeOS right now:
1) there was no other VEA API implementation at the time it was created; so this
was solely to save compile time on bots;
2) no one added CreateAndroidVEA() and ran it on Android yet (which I believe
should've be done when AVEA was added); and if any other VEA impl for any other
platform is created later, it should be added to the test as well;

To reiterate, there is nothing ChromeOS-specific, or VAAPI-specific, or
V4L2-specific in the VEA unittest. It tests a cross-platform VEA API. If I am a
developer/tester coming to the codebase in need for a unittest for VEA API, and
see veatest_chromeos, I think, "Oh, this can't be used outside of chromeos,
there must be a good reason why the author limited it to chromeos only and
explicitly called it _chromeos". (Also, I have no idea why.) Which in fact
wouldn't be so.

If we didn't have the _chromeos suffix, that hypothetical person could say, "Oh
great, looks like they are only using/compiling it for cros right now, but there
doesn't seem to be any reason why I couldn't use it elsewhere if I need to. It
supports the cross-platform VEA API. Great, I just need to modify the build
scripts."

Otherwise, someone may come and write a separate veaunittest_win,
veaunittest_android, etc., which would not be preferable. By design, the
unittest was written to exercise a cross-platform API and there is nothing in it
making it ChromeOS-specific.

[mcasas, 2015-12-02 20:02:46]

On 2015/11/26 01:56:51, Pawel Osciak wrote:
> On 2015/11/25 17:34:16, mcasas wrote:
> > On 2015/11/25 01:03:43, Pawel Osciak wrote:
> > > On 2015/11/25 01:01:40, Pawel Osciak wrote:
> > > > I like the change apart from renaming the vea unittest. It should be
> usable
> > on
> > > > other platforms as well...
> > > 
> > > Specifically, FakeVideoEncodeAccelerator is usable on any platform, and
> > VaapiVEA
> > > should be usable on any Linux.
> > 
> > Chromium is based on the law of demeter, or the minimal
> > needed knowledge: e.g. class definitions are limited to what 
> > is used on ToT, a new pixel format is only introduced if it's 
> > used, etc.
> > In that line of reasoning, the fact that an api, or
> > a file, or class, is potentially usable in several platforms is 
> > irrelevant versus the use in Chromium as of ToT. Think about
> > a developer/tester coming to the codebase, wouldn't they 
> > appreciate the suffixes indicating where the file is _used_?
> 
> 
> I'm sorry, my previous reply was not reflecting the issue at all. Please allow
> me to retract that and correct myself.
> 
> It should actually not be about VAAPI or V4L2 API being potentially usable on
> other platforms, it's about VEA API being cross-platform and this being a test
> for VEA API, not the platform-specific implementations.
> 
> VEA unittest is a test for the platform-independent VEA API. The test *only*
> calls VEA API, not VAAPI, not V4L2, etc., and has no specifics of these two
> platform-specific APIs. Anyone can come, and, for example, add a
> VEAClient::CreateAndroidVEA() method and use it on Android VEA, and the test
> will work with it. VEA is a cross-platform API used not only on ChromeOS. I'm
> actually surprised we don't have this CreateAndroidVEA(), looks like an
> omission.
> 
> There are two reasons why it is compiled only on ChromeOS right now:
> 1) there was no other VEA API implementation at the time it was created; so
this
> was solely to save compile time on bots;
> 2) no one added CreateAndroidVEA() and ran it on Android yet (which I believe
> should've be done when AVEA was added); and if any other VEA impl for any
other
> platform is created later, it should be added to the test as well;
> 
> To reiterate, there is nothing ChromeOS-specific, or VAAPI-specific, or
> V4L2-specific in the VEA unittest. It tests a cross-platform VEA API. If I am
a
> developer/tester coming to the codebase in need for a unittest for VEA API,
and
> see veatest_chromeos, I think, "Oh, this can't be used outside of chromeos,
> there must be a good reason why the author limited it to chromeos only and
> explicitly called it _chromeos". (Also, I have no idea why.) Which in fact
> wouldn't be so.
> 
> If we didn't have the _chromeos suffix, that hypothetical person could say,
"Oh
> great, looks like they are only using/compiling it for cros right now, but
there
> doesn't seem to be any reason why I couldn't use it elsewhere if I need to. It
> supports the cross-platform VEA API. Great, I just need to modify the build
> scripts."
> 
> Otherwise, someone may come and write a separate veaunittest_win,
> veaunittest_android, etc., which would not be preferable. By design, the
> unittest was written to exercise a cross-platform API and there is nothing in
it
> making it ChromeOS-specific.

All right, keep video_encode_accelerator_unittest.cc. Anything else?

[Pawel Osciak, 2015-12-03 06:03:12]

On 2015/12/02 20:02:46, mcasas wrote:
> On 2015/11/26 01:56:51, Pawel Osciak wrote:
> > On 2015/11/25 17:34:16, mcasas wrote:
> > > On 2015/11/25 01:03:43, Pawel Osciak wrote:
> > > > On 2015/11/25 01:01:40, Pawel Osciak wrote:
> > > > > I like the change apart from renaming the vea unittest. It should be
> > usable
> > > on
> > > > > other platforms as well...
> > > > 
> > > > Specifically, FakeVideoEncodeAccelerator is usable on any platform, and
> > > VaapiVEA
> > > > should be usable on any Linux.
> > > 
> > > Chromium is based on the law of demeter, or the minimal
> > > needed knowledge: e.g. class definitions are limited to what 
> > > is used on ToT, a new pixel format is only introduced if it's 
> > > used, etc.
> > > In that line of reasoning, the fact that an api, or
> > > a file, or class, is potentially usable in several platforms is 
> > > irrelevant versus the use in Chromium as of ToT. Think about
> > > a developer/tester coming to the codebase, wouldn't they 
> > > appreciate the suffixes indicating where the file is _used_?
> > 
> > 
> > I'm sorry, my previous reply was not reflecting the issue at all. Please
allow
> > me to retract that and correct myself.
> > 
> > It should actually not be about VAAPI or V4L2 API being potentially usable
on
> > other platforms, it's about VEA API being cross-platform and this being a
test
> > for VEA API, not the platform-specific implementations.
> > 
> > VEA unittest is a test for the platform-independent VEA API. The test *only*
> > calls VEA API, not VAAPI, not V4L2, etc., and has no specifics of these two
> > platform-specific APIs. Anyone can come, and, for example, add a
> > VEAClient::CreateAndroidVEA() method and use it on Android VEA, and the test
> > will work with it. VEA is a cross-platform API used not only on ChromeOS.
I'm
> > actually surprised we don't have this CreateAndroidVEA(), looks like an
> > omission.
> > 
> > There are two reasons why it is compiled only on ChromeOS right now:
> > 1) there was no other VEA API implementation at the time it was created; so
> this
> > was solely to save compile time on bots;
> > 2) no one added CreateAndroidVEA() and ran it on Android yet (which I
believe
> > should've be done when AVEA was added); and if any other VEA impl for any
> other
> > platform is created later, it should be added to the test as well;
> > 
> > To reiterate, there is nothing ChromeOS-specific, or VAAPI-specific, or
> > V4L2-specific in the VEA unittest. It tests a cross-platform VEA API. If I
am
> a
> > developer/tester coming to the codebase in need for a unittest for VEA API,
> and
> > see veatest_chromeos, I think, "Oh, this can't be used outside of chromeos,
> > there must be a good reason why the author limited it to chromeos only and
> > explicitly called it _chromeos". (Also, I have no idea why.) Which in fact
> > wouldn't be so.
> > 
> > If we didn't have the _chromeos suffix, that hypothetical person could say,
> "Oh
> > great, looks like they are only using/compiling it for cros right now, but
> there
> > doesn't seem to be any reason why I couldn't use it elsewhere if I need to.
It
> > supports the cross-platform VEA API. Great, I just need to modify the build
> > scripts."
> > 
> > Otherwise, someone may come and write a separate veaunittest_win,
> > veaunittest_android, etc., which would not be preferable. By design, the
> > unittest was written to exercise a cross-platform API and there is nothing
in
> it
> > making it ChromeOS-specific.
> 
> All right, keep video_encode_accelerator_unittest.cc. Anything else?

Thanks! I'm ok with other changes.

[mcasas, 2015-12-03 19:13:17]

Description was changed from

==========
content/common/gpu/media add platform suffix to some files (cleanup)

to make more evident which file is in which platform,
plus they get automatically removed from build lists
in the appropriate way.

BUG=552520
==========

to

==========
content/common/gpu/media add platform suffix to some files (cleanup)

to make more evident which file is in which platform,
plus they get automatically removed from build lists
in the appropriate way.

BUG=

TBR=avi@chromium.org for the changes not covered by
sandersd@ and posciak@ :
- content/common/BUILD.gn
- content/common/sandbox_mac.mm
- content/gpugpu_main.cc
 (those are just renaming of the appropriate files).
==========

[mcasas, 2015-12-03 19:49:43]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2015-12-03 19:49:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1474823002/#ps40001
(title: "Reverted naming video_encode_accelerator_unittest.cc; A few
content/content_tests.gypi alphabetical…")

[commit-bot: I haz the power, 2015-12-03 19:50:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1474823002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1474823002/40001

[commit-bot: I haz the power, 2015-12-03 21:15:19]

Description was changed from

==========
content/common/gpu/media add platform suffix to some files (cleanup)

to make more evident which file is in which platform,
plus they get automatically removed from build lists
in the appropriate way.

BUG=

TBR=avi@chromium.org for the changes not covered by
sandersd@ and posciak@ :
- content/common/BUILD.gn
- content/common/sandbox_mac.mm
- content/gpugpu_main.cc
 (those are just renaming of the appropriate files).
==========

to

==========
content/common/gpu/media add platform suffix to some files (cleanup)

to make more evident which file is in which platform,
plus they get automatically removed from build lists
in the appropriate way.

BUG=

TBR=avi@chromium.org for the changes not covered by
sandersd@ and posciak@ :
- content/common/BUILD.gn
- content/common/sandbox_mac.mm
- content/gpugpu_main.cc
 (those are just renaming of the appropriate files).
==========

[commit-bot: I haz the power, 2015-12-03 21:15:20]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-12-03 21:17:01]

Description was changed from

==========
content/common/gpu/media add platform suffix to some files (cleanup)

to make more evident which file is in which platform,
plus they get automatically removed from build lists
in the appropriate way.

BUG=

TBR=avi@chromium.org for the changes not covered by
sandersd@ and posciak@ :
- content/common/BUILD.gn
- content/common/sandbox_mac.mm
- content/gpugpu_main.cc
 (those are just renaming of the appropriate files).
==========

to

==========
content/common/gpu/media add platform suffix to some files (cleanup)

to make more evident which file is in which platform,
plus they get automatically removed from build lists
in the appropriate way.

BUG=

TBR=avi@chromium.org for the changes not covered by
sandersd@ and posciak@ :
- content/common/BUILD.gn
- content/common/sandbox_mac.mm
- content/gpugpu_main.cc
 (those are just renaming of the appropriate files).

Committed: https://crrev.com/7479738f45a79b6262a08be4e81e901e519fcb30
Cr-Commit-Position: refs/heads/master@{#363061}
==========

[commit-bot: I haz the power, 2015-12-03 21:17:02]

Patchset 2 (id:??) landed as
https://crrev.com/7479738f45a79b6262a08be4e81e901e519fcb30
Cr-Commit-Position: refs/heads/master@{#363061}