Rename functions in video frame to comply with Style Guide

Rename functions in video frame to comply with Style Guide

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2781283002
Cr-Commit-Position: refs/heads/master@{#463153}
Committed: https://chromium.googlesource.com/chromium/src/+/46944f80bfc35a8589514f8b8c9d895c21e17f2a

[Uzair, 2017-03-30 05:44:37]

Description was changed from

==========
Rename functions in video frame to comply with Style Guide

BUG=None
==========

to

==========
Rename functions in video frame to comply with Style Guide

BUG=None
==========

[Uzair, 2017-03-30 05:44:37]

uzair.jaleel@samsung.com changed reviewers:
+ a.suchit@chromium.org, mcasas@chromium.org

[Uzair, 2017-03-30 05:44:50]

On 2017/03/30 05:44:37, Uzair wrote:
> mailto:uzair.jaleel@samsung.com changed reviewers:
> + mailto:a.suchit@chromium.org, mailto:mcasas@chromium.org

PTAL

[mcasas, 2017-03-30 17:30:23]

mcasas@chromium.org changed reviewers:
+ chfremer@chromium.org

[mcasas, 2017-03-30 17:31:10]

On 2017/03/30 05:44:50, Uzair wrote:
> On 2017/03/30 05:44:37, Uzair wrote:
> > mailto:uzair.jaleel@samsung.com changed reviewers:
> > + mailto:a.suchit@chromium.org, mailto:mcasas@chromium.org
> 
> PTAL

Please update description since you also change
STORAGE_DMABUF to STORAGE_NATIVE. 
Otherwise doesn't look bad, adding chfremer@ for
further review.

[chfremer, 2017-03-30 17:55:44]

I am not an OWNER, but I am happy to give some feedback/raise some questions.

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

https://codereview.chromium.org/2781283002/diff/1/media/base/video_frame.h#ne...
media/base/video_frame.h:66: STORAGE_NATIVE = 5,  // Each plane is stored into a
DmaBuf.
Why is STORAGE_NATIVE a more appropriate name than STORAGE_DMABUFS? It seems
less specific. Are there cases where this is used with non-DMA buffers?

https://codereview.chromium.org/2781283002/diff/1/media/base/video_frame.h#ne...
media/base/video_frame.h:340: int DmabufFd(size_t plane) const;
If we do not want to be as specific as saying "DMA buffers" in the enum value
above, should we also make the name of this method more general?

[mcasas, 2017-03-30 18:58:14]

The CQ bit was checked by mcasas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 18:58:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 19:18:12]

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

[commit-bot: I haz the power, 2017-03-30 19:18:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Uzair, 2017-03-31 10:04:52]

Description was changed from

==========
Rename functions in video frame to comply with Style Guide

BUG=None
==========

to

==========
Rename functions in video frame to comply with Style Guide

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Uzair, 2017-03-31 10:09:19]

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

https://codereview.chromium.org/2781283002/diff/1/media/base/video_frame.h#ne...
media/base/video_frame.h:66: STORAGE_NATIVE = 5,  // Each plane is stored into a
DmaBuf.
On 2017/03/30 17:55:44, chfremer wrote:
> Why is STORAGE_NATIVE a more appropriate name than STORAGE_DMABUFS? It seems
> less specific. Are there cases where this is used with non-DMA buffers? 

Updated the name as per comment mentioned, but yes your right STORAGE_DMABUFS
looks better as i am not aware of any cases where this is used with non-DMA
buffers.

https://codereview.chromium.org/2781283002/diff/1/media/base/video_frame.h#ne...
media/base/video_frame.h:340: int DmabufFd(size_t plane) const;
On 2017/03/30 17:55:44, chfremer wrote:
> If we do not want to be as specific as saying "DMA buffers" in the enum value
> above, should we also make the name of this method more general?

Changed above as i am not aware of any Non DMA buffer cases where it might be
used.

[Uzair, 2017-03-31 10:09:42]

The CQ bit was checked by uzair.jaleel@samsung.com to run a CQ dry run

[Uzair, 2017-03-31 10:09:52]

The CQ bit was unchecked by uzair.jaleel@samsung.com

[commit-bot: I haz the power, 2017-03-31 10:09:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 10:09:53]

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

[commit-bot: I haz the power, 2017-03-31 10:09:54]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[Uzair, 2017-03-31 10:10:45]

On 2017/03/31 10:09:19, Uzair wrote:
> https://codereview.chromium.org/2781283002/diff/1/media/base/video_frame.h
> File media/base/video_frame.h (right):
> 
>
https://codereview.chromium.org/2781283002/diff/1/media/base/video_frame.h#ne...
> media/base/video_frame.h:66: STORAGE_NATIVE = 5,  // Each plane is stored into
a
> DmaBuf.
> On 2017/03/30 17:55:44, chfremer wrote:
> > Why is STORAGE_NATIVE a more appropriate name than STORAGE_DMABUFS? It seems
> > less specific. Are there cases where this is used with non-DMA buffers? 
> 
> Updated the name as per comment mentioned, but yes your right STORAGE_DMABUFS
> looks better as i am not aware of any cases where this is used with non-DMA
> buffers.
> 
>
https://codereview.chromium.org/2781283002/diff/1/media/base/video_frame.h#ne...
> media/base/video_frame.h:340: int DmabufFd(size_t plane) const;
> On 2017/03/30 17:55:44, chfremer wrote:
> > If we do not want to be as specific as saying "DMA buffers" in the enum
value
> > above, should we also make the name of this method more general?
> 
> Changed above as i am not aware of any Non DMA buffer cases where it might be
> used.

PTAL

Thanks

[chfremer, 2017-03-31 17:50:21]

lgtm % one optional suggestion

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

https://codereview.chromium.org/2781283002/diff/20001/media/base/video_frame....
media/base/video_frame.h:66: STORAGE_DMABUFS = 5,  // Each plane is stored into
a DmaBuf.
I chatted with mcasas@ and it seems that his suggestion to rename this to
STORAGE_NATIVE was based on the idea of using this same enum value for both DMA
buffers on Linux and CVPixelBuffers on Mac (which currently use
STORAGE_UNOWNED_MEMORY).

Maybe, instead of dropping the TODO here, we could reformulate it to express
that.

[Uzair, 2017-04-03 04:17:40]

Done.

PTAL

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

https://codereview.chromium.org/2781283002/diff/20001/media/base/video_frame....
media/base/video_frame.h:66: STORAGE_DMABUFS = 5,  // Each plane is stored into
a DmaBuf.
On 2017/03/31 17:50:21, chfremer wrote:
> I chatted with mcasas@ and it seems that his suggestion to rename this to
> STORAGE_NATIVE was based on the idea of using this same enum value for both
DMA
> buffers on Linux and CVPixelBuffers on Mac (which currently use
> STORAGE_UNOWNED_MEMORY).
> 
> Maybe, instead of dropping the TODO here, we could reformulate it to express
> that.

Done.

[Uzair, 2017-04-04 10:13:59]

On 2017/04/03 04:17:40, Uzair wrote:
> Done.
> 
> PTAL
> 
> https://codereview.chromium.org/2781283002/diff/20001/media/base/video_frame.h
> File media/base/video_frame.h (right):
> 
>
https://codereview.chromium.org/2781283002/diff/20001/media/base/video_frame....
> media/base/video_frame.h:66: STORAGE_DMABUFS = 5,  // Each plane is stored
into
> a DmaBuf.
> On 2017/03/31 17:50:21, chfremer wrote:
> > I chatted with mcasas@ and it seems that his suggestion to rename this to
> > STORAGE_NATIVE was based on the idea of using this same enum value for both
> DMA
> > buffers on Linux and CVPixelBuffers on Mac (which currently use
> > STORAGE_UNOWNED_MEMORY).
> > 
> > Maybe, instead of dropping the TODO here, we could reformulate it to express
> > that.
> 
> Done.

Dear Reviewers,

PTAL

[mcasas, 2017-04-04 16:25:06]

lgtm

[mcasas, 2017-04-04 16:25:16]

The CQ bit was checked by mcasas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 16:25:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 16:45:20]

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

[commit-bot: I haz the power, 2017-04-04 16:45:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Uzair, 2017-04-05 04:30:50]

The CQ bit was checked by uzair.jaleel@samsung.com

[Uzair, 2017-04-05 04:30:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
chfremer@chromium.org, mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2781283002/#ps60001
(title: "Fixed compilation errors for MAC")

[commit-bot: I haz the power, 2017-04-05 04:31:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 04:37:50]

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

[commit-bot: I haz the power, 2017-04-05 04:37:51]

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

[Uzair, 2017-04-05 07:52:35]

On 2017/04/05 04:37:51, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Dear reviewers

There was build error which was fixed. 
Please review media/cast/sender/h264_vt_encoder_unittest.cc

Thanks

[Uzair, 2017-04-05 10:26:14]

Description was changed from

==========
Rename functions in video frame to comply with Style Guide

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Rename functions in video frame to comply with Style Guide

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Uzair, 2017-04-05 10:26:14]

uzair.jaleel@samsung.com changed reviewers:
+ hubbe@chromium.org

[chfremer, 2017-04-05 16:23:27]

On 2017/04/05 07:52:35, Uzair wrote:
> On 2017/04/05 04:37:51, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> Dear reviewers
> 
> There was build error which was fixed. 
> Please review media/cast/sender/h264_vt_encoder_unittest.cc
> 
> Thanks

In general, there is no need to ask reviewers to re-confirm their sign-offs if
the changes since 
their sign-off are canonical, as is the case here.

The reason the commit failed is that neither mcasas@ nor myself are OWNERs of
the files touched in this CL.

[Uzair, 2017-04-06 09:37:39]

On 2017/04/05 16:23:27, chfremer wrote:
> On 2017/04/05 07:52:35, Uzair wrote:
> > On 2017/04/05 04:37:51, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> > 
> > Dear reviewers
> > 
> > There was build error which was fixed. 
> > Please review media/cast/sender/h264_vt_encoder_unittest.cc
> > 
> > Thanks
> 
> In general, there is no need to ask reviewers to re-confirm their sign-offs if
> the changes since 
> their sign-off are canonical, as is the case here.
> 
> The reason the commit failed is that neither mcasas@ nor myself are OWNERs of
> the files touched in this CL.

Dear Hubbe,

PTAL

Thanks

[hubbe, 2017-04-06 18:56:36]

lgtm

[Uzair, 2017-04-07 04:00:03]

The CQ bit was checked by uzair.jaleel@samsung.com

[commit-bot: I haz the power, 2017-04-07 04:00:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 06:42:20]

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

[commit-bot: I haz the power, 2017-04-07 06:42:21]

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

[Uzair, 2017-04-07 16:08:32]

On 2017/04/07 06:42:21, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)

Hi ,

Not sure why webgl conformance test is failing with this patch

Any input would be helpful 

Thanks

[chfremer, 2017-04-07 17:00:43]

On 2017/04/07 16:08:32, Uzair wrote:
> On 2017/04/07 06:42:21, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   linux_optional_gpu_tests_rel on master.tryserver.chromium.linux
(JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
> 
> Hi ,
> 
> Not sure why webgl conformance test is failing with this patch
> 
> Any input would be helpful 
> 
> Thanks

I don't think it fails because of the changes in this CL.
Looking at [1] it seems like it is currently in a bad state.
I recommend we wait a bit to see if it clears, or maybe try to find a sheriff
and ask about it.

[1]
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional...

[Uzair, 2017-04-09 12:54:26]

The CQ bit was checked by uzair.jaleel@samsung.com

[commit-bot: I haz the power, 2017-04-09 12:54:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-09 22:00:11]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491742466653900,
"parent_rev": "eb78b6f967fa965906d28a29190a89cce95190ec", "commit_rev":
"46944f80bfc35a8589514f8b8c9d895c21e17f2a"}

[commit-bot: I haz the power, 2017-04-09 22:01:03]

Description was changed from

==========
Rename functions in video frame to comply with Style Guide

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Rename functions in video frame to comply with Style Guide

BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2781283002
Cr-Commit-Position: refs/heads/master@{#463153}
Committed:
https://chromium.googlesource.com/chromium/src/+/46944f80bfc35a8589514f8b8c9d...
==========

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

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/46944f80bfc35a8589514f8b8c9d...