vda: Rename Exynos VDA to V4L2 VDA

vda: Rename Exynos VDA to V4L2 VDA

This change is the first pass of restructuring
the exynos vda implementation to a more generic
v4l2 vda to facilitate support for other v4l2
based vda.

BUG=chromium-os-partner:24752
TEST=Run video playback, vda_unittest

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243803

[shivdasp, 2014-01-06 09:41:30]

ping.

[Pawel Osciak, 2014-01-06 09:51:31]

On 2014/01/06 09:41:30, shivdasp wrote:
> ping.

Oh sorry. I didn't get the notification email when you first uploaded this.
Please "Publish+Mail Comments" when you upload/update a CL.

[Pawel Osciak, 2014-01-06 09:53:30]

On 2014/01/06 09:51:31, posciak wrote:
> On 2014/01/06 09:41:30, shivdasp wrote:
> > ping.
> 
> Oh sorry. I didn't get the notification email when you first uploaded this.
> Please "Publish+Mail Comments" when you upload/update a CL.

Could you please update the previous issue
(https://codereview.chromium.org/104713014/) with this CL instead of creating a
new one? That way we could see a diff between this and previous patchset and
follow up on comments from the previous iteration.

[shivdasp, 2014-01-06 10:05:19]

On 2014/01/06 09:53:30, posciak wrote:
> On 2014/01/06 09:51:31, posciak wrote:
> > On 2014/01/06 09:41:30, shivdasp wrote:
> > > ping.
> > 
> > Oh sorry. I didn't get the notification email when you first uploaded this.
> > Please "Publish+Mail Comments" when you upload/update a CL.
> 
> Could you please update the previous issue
> (https://codereview.chromium.org/104713014/) with this CL instead of creating
a
> new one? That way we could see a diff between this and previous patchset and
> follow up on comments from the previous iteration.

The other one was created by my colleague. Is there a way I can create a
patchset on an existing CL ?
Based on the review comments on the earlier CL I though I'd break up all the
changes in smaller patchsets for ease of review/merge.

[Pawel Osciak, 2014-01-06 10:11:01]

On 2014/01/06 10:05:19, shivdasp wrote:
> On 2014/01/06 09:53:30, posciak wrote:
> > On 2014/01/06 09:51:31, posciak wrote:
> > > On 2014/01/06 09:41:30, shivdasp wrote:
> > > > ping.
> > > 
> > > Oh sorry. I didn't get the notification email when you first uploaded
this.
> > > Please "Publish+Mail Comments" when you upload/update a CL.
> > 
> > Could you please update the previous issue
> > (https://codereview.chromium.org/104713014/) with this CL instead of
creating
> a
> > new one? That way we could see a diff between this and previous patchset and
> > follow up on comments from the previous iteration.
> 
> The other one was created by my colleague. Is there a way I can create a
> patchset on an existing CL ?

Yes, you should be able to. You just need to set the correct issue number. From
the branch you created this issue from please say:
git cl issue 0
git cl issue 104713014
git cl upload

[shivdasp, 2014-01-06 10:27:15]

On 2014/01/06 10:11:01, posciak wrote:
> On 2014/01/06 10:05:19, shivdasp wrote:
> > On 2014/01/06 09:53:30, posciak wrote:
> > > On 2014/01/06 09:51:31, posciak wrote:
> > > > On 2014/01/06 09:41:30, shivdasp wrote:
> > > > > ping.
> > > > 
> > > > Oh sorry. I didn't get the notification email when you first uploaded
> this.
> > > > Please "Publish+Mail Comments" when you upload/update a CL.
> > > 
> > > Could you please update the previous issue
> > > (https://codereview.chromium.org/104713014/) with this CL instead of
> creating
> > a
> > > new one? That way we could see a diff between this and previous patchset
and
> > > follow up on comments from the previous iteration.
> > 
> > The other one was created by my colleague. Is there a way I can create a
> > patchset on an existing CL ?
> 
> Yes, you should be able to. You just need to set the correct issue number.
From
> the branch you created this issue from please say:
> git cl issue 0
> git cl issue 104713014
> git cl upload

I got below error:
"Issue creation errors: {'user': ["You (shivdasp@nvidia.com) don't own this
issue (104713014)"]}"
Can ownership be transferred ?

[Ami GONE FROM CHROMIUM, 2014-01-06 16:20:45]

On Mon, Jan 6, 2014 at 2:27 AM, <shivdasp@nvidia.com> wrote:

> I got below error:
> "Issue creation errors: {'user': ["You (shivdasp@nvidia.com) don't own
> this
> issue (104713014)"]}"
> Can ownership be transferred ?
>

I don't think so.
You'll need to get the issue owner to upload your updated patchset (by
patching it in locally).

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

[Pawel Osciak, 2014-01-07 04:03:04]

On 2014/01/06 16:20:45, Ami Fischman wrote:
> On Mon, Jan 6, 2014 at 2:27 AM, <mailto:shivdasp@nvidia.com> wrote:
> 
> > I got below error:
> > "Issue creation errors: {'user': ["You (mailto:shivdasp@nvidia.com) don't
own
> > this
> > issue (104713014)"]}"
> > Can ownership be transferred ?
> >
> 
> I don't think so.
> You'll need to get the issue owner to upload your updated patchset (by
> patching it in locally).

Thanks Ami. After looking at the contents, this CL is not really that related to
the previous one after all, this is just a rename. CL description is a bit
misleading.
So let's continue here.

[Pawel Osciak, 2014-01-07 04:16:26]

Comments added. Please also update the CL description to something like "Rename
Exynos VDA to V4L2 VDA" instead, since there's not really any restructuring done
here.. And please drop the "chromium" prefix.

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:193:
decoder_thread_("ExynosDecoderThread"),
s/ExynosDecoderThread/DecoderThread/

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:211:
device_poll_thread_("ExynosDevicePollThread"),
DevicePollThread

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:287: DPLOG(ERROR) <<
"Initialize(): could not open MFC device: "
MFC -> decoder

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
File content/common/gpu/media/v4l2_video_decode_accelerator.h (right):

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
content/common/gpu/media/v4l2_video_decode_accelerator.h:6: // that utilizes the
hardware video decoder present on the Exynos SoC.
There are two remaining mentions of "Exynos" in this file. Let's get rid of them
as well and update the docs.

Let's say something like:

"This file contains an implementation of VideoDecodeAccelerator
that utilizes hardware video decoders, which expose Video4Linux 2 API
(http://linuxtv.org/downloads/v4l-dvb-apis/)."

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
content/common/gpu/media/v4l2_video_decode_accelerator.h:34: // device exported
by the Multi Format Codec hardware block.
"This class handles video accelerators directly through a V4L2 device exported
by the hardware blocks."

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
content/common/gpu/media/v4l2_video_decode_accelerator.h:59: class
CONTENT_EXPORT V4l2VideoDecodeAccelerator
Nit: I think we might prefer V4L2 (with capital "L").

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
content/common/gpu/media/v4l2_video_decode_accelerator.h:87:
kMfcInputBufferCount = 8,
I'm leaning towards renaming MFC -> Decoder everywhere.

https://codereview.chromium.org/122233002/diff/1/content/content_common.gypi
File content/content_common.gypi (right):

https://codereview.chromium.org/122233002/diff/1/content/content_common.gypi#...
content/content_common.gypi:550:
'common/gpu/media/v4l2_video_decode_accelerator.cc',
Please move this down to maintain lexicographical order.

[shivdasp, 2014-01-07 04:31:13]

Will address all the comments.
In general will remove all references to exynos and mfc from these files.

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:193:
decoder_thread_("ExynosDecoderThread"),
On 2014/01/07 04:16:27, posciak wrote:
> s/ExynosDecoderThread/DecoderThread/

Done.

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:211:
device_poll_thread_("ExynosDevicePollThread"),
On 2014/01/07 04:16:27, posciak wrote:
> DevicePollThread

Done.

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
File content/common/gpu/media/v4l2_video_decode_accelerator.h (right):

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
content/common/gpu/media/v4l2_video_decode_accelerator.h:59: class
CONTENT_EXPORT V4l2VideoDecodeAccelerator
On 2014/01/07 04:16:27, posciak wrote:
> Nit: I think we might prefer V4L2 (with capital "L").

Done.

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
content/common/gpu/media/v4l2_video_decode_accelerator.h:87:
kMfcInputBufferCount = 8,
Should we replace "MFC" with "Decoder" for members too (mfc_input_stream_on_
etc..) ? And is the Decoder really required since this file is the decoder impl.
Names would become:
kMfcInputBufferCount -> kDecoderInputBufferCount
Just kInputBufferCount will do ? Variable names will also become very long.

On 2014/01/07 04:16:27, posciak wrote:
> I'm leaning towards renaming MFC -> Decoder everywhere.

[Pawel Osciak, 2014-01-07 04:35:48]

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
File content/common/gpu/media/v4l2_video_decode_accelerator.h (right):

https://codereview.chromium.org/122233002/diff/1/content/common/gpu/media/v4l...
content/common/gpu/media/v4l2_video_decode_accelerator.h:87:
kMfcInputBufferCount = 8,
On 2014/01/07 04:31:14, shivdasp wrote:
> Should we replace "MFC" with "Decoder" for members too (mfc_input_stream_on_
> etc..) ? And is the Decoder really required since this file is the decoder
impl.
> Names would become:
> kMfcInputBufferCount -> kDecoderInputBufferCount
> Just kInputBufferCount will do ? Variable names will also become very long.
> 

Good point. I agree.

[shivdasp, 2014-01-07 06:58:35]

There are a couple of references for Exynos in comments since the description is
specific for it. In subsequent CLs for restructuring they might move to
different file altogether.

[Pawel Osciak, 2014-01-07 07:18:12]

Just a few more nits. Thanks for your patience!

https://codereview.chromium.org/122233002/diff/210001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/122233002/diff/210001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:284: fd_ =
HANDLE_EINTR(open(kDevice,
This and below might fit in one line now.

https://codereview.chromium.org/122233002/diff/210001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:319: // output format
has to be setup before streaming starts.
Capital letter.

https://codereview.chromium.org/122233002/diff/210001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:1175: // Enqueue a
input (VIDEO_OUTPUT) buffer.
s/a/an/

https://codereview.chromium.org/122233002/diff/210001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:1203: // Enqueue a
output (VIDEO_CAPTURE) buffer.
s/a/an/

https://codereview.chromium.org/122233002/diff/210001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:1378: // We stop
streaming and clear buffer tracking info (not preserving
Wrap line please.

https://codereview.chromium.org/122233002/diff/210001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:1668: bool
event_pending = (pollfd != -1 &&
Line wrap please.

https://codereview.chromium.org/122233002/diff/210001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_decode_accelerator.h (right):

https://codereview.chromium.org/122233002/diff/210001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.h:117: kPoll = (1 << 0),
This doesn't make too much sense now. Maybe kPollDecoder?

https://codereview.chromium.org/122233002/diff/210001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.h:365: // decode device.
Comments should start with capital letters (be sentences in general).
Here and everywhere else.

[shivdasp, 2014-01-07 07:48:55]

Incorporated review comments. Please have a look.

[Pawel Osciak, 2014-01-07 08:06:37]

Alright, this lgtm to me with 4 remaining small comments.

I'll ask Ami for a final look.

CC sheu as FYI.

https://codereview.chromium.org/122233002/diff/280001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/122233002/diff/280001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:913: // Enqueue since
there's new available input for it.
"once" missing from the original comment.

https://codereview.chromium.org/122233002/diff/280001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:989: // can be polled
as soon as either input or output buffers are queued.
Capital letter.

https://codereview.chromium.org/122233002/diff/280001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:1319: // * input
holding queue is empty.
Capital letter please.

https://codereview.chromium.org/122233002/diff/280001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_decode_accelerator.h (right):

https://codereview.chromium.org/122233002/diff/280001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.h:334: // input buffer
we're presently filling.
Capital letter please.

[shivdasp, 2014-01-07 08:53:21]

Review comments incorporated.

Pawel,
I plan to upload another CL for introducing the v4l2_device abstraction and real
restructuring or do you want me to upload in
https://codereview.chromium.org/104713014/.
I prefer another CL since the other is owned by vpagar and it'll be cumbersome
to get these changes uploaded. (will have to get his environment every time.)

[Ami GONE FROM CHROMIUM, 2014-01-08 01:56:38]

LGTM % nits.
Thanks for the cleanup!

https://codereview.chromium.org/122233002/diff/350001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/122233002/diff/350001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:193:
decoder_thread_("DecoderThread"),
s/DecoderThread/V4L2DecoderThread/ please

https://codereview.chromium.org/122233002/diff/350001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:211:
device_poll_thread_("DevicePollThread"),
s/DevicePollThread/V4L2DevicePollThread/ please

https://codereview.chromium.org/122233002/diff/350001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_decode_accelerator.h (right):

https://codereview.chromium.org/122233002/diff/350001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.h:9: #ifndef
CONTENT_COMMON_GPU_MEDIA_V4L2_VIDEO_DECODE_ACCELERATOR_H_
Please update the files in content/common/gpu/testdata/*README that refer to
Exynos/EVDA to instead refer to V4L2VDA.

[Pawel Osciak, 2014-01-08 01:58:58]

On 2014/01/07 08:53:21, shivdasp wrote:
> Review comments incorporated.
> 
> Pawel,
> I plan to upload another CL for introducing the v4l2_device abstraction and
real
> restructuring or do you want me to upload in
> https://codereview.chromium.org/104713014/.
> I prefer another CL since the other is owned by vpagar and it'll be cumbersome
> to get these changes uploaded. (will have to get his environment every time.)

Sure, in this situation let's go with a new CL. Thanks!

[Pawel Osciak, 2014-01-08 02:00:20]

piman@: Please OWNERS for content_commoon

[piman, 2014-01-08 02:22:21]

lgtm

https://codereview.chromium.org/122233002/diff/350001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/122233002/diff/350001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:59: const char
kDevice[] = "/dev/mfc-dec";
mmh... renaming doesn't make it less exynos-specific :)

[Pawel Osciak, 2014-01-08 02:27:55]

https://codereview.chromium.org/122233002/diff/350001/content/common/gpu/medi...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/122233002/diff/350001/content/common/gpu/medi...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:59: const char
kDevice[] = "/dev/mfc-dec";
On 2014/01/08 02:22:21, piman (OOO back 2014-1-7) wrote:
> mmh... renaming doesn't make it less exynos-specific :)

Of course not :)
This is just a first CL of the series. The following ones will abstract this.

[shivdasp, 2014-01-08 05:14:44]

Thanks for review everyone. Please have a look at new patchset.

[Ami GONE FROM CHROMIUM, 2014-01-08 21:21:41]

PS#5 still LGTM.

[Pawel Osciak, 2014-01-09 01:23:34]

PS5 lgtm from me as well.

[shivdasp, 2014-01-09 05:11:40]

Any action on me now ?

[Pawel Osciak, 2014-01-09 05:13:00]

On 2014/01/09 05:11:40, shivdasp wrote:
> Any action on me now ?

Putting a checkmark in the checkbox next to "Commit:" above :)
I'll do it for you.

[commit-bot: I haz the power, 2014-01-09 05:13:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shivdasp@nvidia.com/122233002/450001

[commit-bot: I haz the power, 2014-01-09 07:46:09]

Change committed as 243803