Issue 104713014: VDA: Restructure EVDA

Issue 104713014: VDA: Restructure EVDA (Closed)

Created:
7 years ago by vpagar

Modified:
6 years, 6 months ago

Reviewers:
shivdasp, Pawel Osciak

CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

VDA: Restructure EVDA This CL restructures EVDA implementation to facilitate easy addition of other V4L2 based implementations. BUG=

Patch Set 1 #

Total comments: 26

Created: 7 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+236 lines, -176 lines)			Patch
M	content/common/gpu/media/exynos_video_decode_accelerator.h	View	8 chunks	+37 lines, -11 lines	15 comments	Download
M	content/common/gpu/media/exynos_video_decode_accelerator.cc	View	76 chunks	+174 lines, -141 lines	10 comments	Download
M	content/common/gpu/media/gpu_video_decode_accelerator.cc	View	8 chunks	+25 lines, -24 lines	1 comment	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

vpagar

This CL restructures EVDA implementation to facilitate easy addition of other V4L2 based implementations.

7 years ago (2013-12-23 13:57:15 UTC) #1

Pawel Osciak

https://codereview.chromium.org/104713014/diff/1/content/common/gpu/media/exynos_video_decode_accelerator.cc File content/common/gpu/media/exynos_video_decode_accelerator.cc (right): https://codereview.chromium.org/104713014/diff/1/content/common/gpu/media/exynos_video_decode_accelerator.cc#newcode36 content/common/gpu/media/exynos_video_decode_accelerator.cc:36: if (HANDLE_EINTR(device->dev_ioctl(fd, type, arg) != 0)) { \ Please ...

6 years, 12 months ago (2013-12-24 03:45:24 UTC) #2

commit-bot: I haz the power

No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an ...

6 years, 12 months ago (2013-12-26 06:12:28 UTC) #4

shivdasp

-----------Copying the email conversation with Pawel regarding this here for reference------------ On Tue, Dec 24, ...

6 years, 12 months ago (2013-12-26 06:14:33 UTC) #5

-----------Copying the email conversation with Pawel regarding this here for
reference------------
On Tue, Dec 24, 2013 at 7:24 PM, Shivdas Patil <shivdasp@nvidia.com> wrote:
> Hi Pawel,
 
> The libtegrav4l2 which we are interfacing with is actually a pseudo v4l2 like
implementation making calls into another library(ies) which . > in turn calls
into the NVAVP driver through a custom proprietary interface (not v4l2).
> This nvavp driver will make its way to the kernel soon.

Great, looking forward to it.
 
> Responding to your review comments here for elaborate explanation and our
proposal.

It would be better to use the Chromium code review site for all design
discussion, if you wouldn't mind please. There are others observing the review
and they may want to comment as well. If they have additional comments, it's
better if they see your proposal early, before you take time to implement. If we
do it over email, there is a possibility someone may later ask to change
something after you've taken time to implement and test.
You can respond in an e-mail style in the review tool interface as well, by
clicking "Reply" on the last message in the "Messages" section, at the bottom of
the main screen for the CL.
 
 
> 1.       Rename exynos_video_decode_accelerator.h to
v4l2_video_decode_accelerator.h and .cc as well.
Yes. 
> 2.       Introduce v4l2_video_ device.h which defines below interface.
> Class v4l2_video_device {
>   V4l2_open(const char *fd,int flags) = 0;

We don't need any arguments here. Device name and flags should be an
implementation detail of a concrete class, especially since those arguments
don't really apply to Tegra for example. Or do you need to pass a device name to
your libraries? How do you interact with them?

>   V4l2_close() = 0;
I think once we open a device, its destructor could take care of closing it. So
probably no need for this.
>  V4l2_ioctl(int flags, void *arg) = 0; //Need ioctl type and argument
>   // I guess there should be a good way to name below two and are more of how
exynos’s polling interrupt mechanism is implemented. On tegra > these are no-ops
but since they are called on the “fd” we have to abstract it out here.
>   V4l2_setdevicepollinterrupt() = 0;

How do you wake up the polling thread in your case if you need to abort it? 

>   V4l2_cleardevicepollinterrupt() = 0;
I suspect this is not really needed even in EVDA, we could clear immediately
after acknowledging the interrupt. But this is something for later that I should
probably fix myself. 
>    V4l2_poll() = 0; //Will move all the fdset creation inside exynosV4L2
device.
>    // Again these 
>    V4l2_mmap(void *addr, unsigned int len, int prot, int flags, unsigned int
offset) = 0;
>    V4l2_munmap(void *addr, unsigned int len) =0;
> Private: 
>   Char device_name_;
>   Int videodec_fd_;
>   Int device_poll_interrupt_fd_;

Do you need all these members for Tegra? I think this should be a pure abstract
class.
> }
> The long list of arugments can’t be avoided (except the fd) since we are
abstracting the video device itself.

I'm guessing you mean arguments to m{,un}map. Yes, I don't see a better solution
for this either right now.

Also, please drop the V4l2_ prefixes from methods and please follow the Chromium
coding style. 
 
> 3.       Introduce a  file exynos_v4l2_video_device.h and
exynos_v4l2_video_device.cc which has implementation for Exynos specific
>V4l2_video_device
> Class exynos_v4l2_video_device :: public v4l2_video_device {
> The implementation of v4l2_xxx () will actually make the system calls the way
they are implemented now.
 
> }
> 4.       Introduce a file tegra_v4l2_video_device.h and
tegra_v4l2_video_device.cc. This will have implementations for Tegra specific
>stuff. It would need the egl_context_ from
GpuVideoDecodeAccelerator::Initialize.

This of course should be a separate CL. 
> And possibly also the library handle since instead of system calls like
open(), close() we would need to make calls into libtegrav4l2.
> That should be an implementation detail of the Tegra device class. 
> Class tegra_v4l2_video_device :: public v4l2_video_device {
> The implementation of v4l2_xxx() will call into the tegra_v4l2_open() function
which the libtegrav4l2 exposes.

> }
>5.       In GpuVideoDecodeAccelerator::Initialize() before creating the
v4l2_video_decode_accelerator object, we would create a >v4l2_video_device and
pass it as an argument to it. The question is how we instantiate an appropriate
one ?
> Can we keep the video device names different on tegra vs exynos ? So if
opening of /dev/mfc-dec fails we instantiate the >tegra_v4l2_video_device. Any
other ideas ?
> That's fine. You could also use VIDIOC_QUERYCAP to get driver name. I think
the simplest way is probably to have the factory method try >each of the classes
until one of them succeeds in opening their device. I'm actually flirting with
an idea of making Open static as well and > returning an object of its class if
it succeeds in opening the device to the factory method. What do you think?
> Is the above class hierarchy okay with you ?

Yes, looks ok.
  
> So basically we will have v4l2_video_decode_accelerator for all the common
logic on exynos & tegra and use v4l2_video_device to do the > device specific
implementation.

That's the idea, yes.
 
 
>Shivdas

shivdasp

On 2013/12/26 06:14:33, shivdasp wrote: > -----------Copying the email conversation with Pawel regarding this here ...

6 years, 12 months ago (2013-12-26 07:03:01 UTC) #6

On 2013/12/26 06:14:33, shivdasp wrote:
> -----------Copying the email conversation with Pawel regarding this here for
> reference------------
> On Tue, Dec 24, 2013 at 7:24 PM, Shivdas Patil <mailto:shivdasp@nvidia.com>
wrote:
> > Hi Pawel,
>  
> > The libtegrav4l2 which we are interfacing with is actually a pseudo v4l2
like
> implementation making calls into another library(ies) which . > in turn calls
> into the NVAVP driver through a custom proprietary interface (not v4l2).
> > This nvavp driver will make its way to the kernel soon.
> 
> Great, looking forward to it.
>  
> > Responding to your review comments here for elaborate explanation and our
> proposal.
> 
> It would be better to use the Chromium code review site for all design
> discussion, if you wouldn't mind please. There are others observing the review
> and they may want to comment as well. If they have additional comments, it's
> better if they see your proposal early, before you take time to implement. If
we
> do it over email, there is a possibility someone may later ask to change
> something after you've taken time to implement and test.
> You can respond in an e-mail style in the review tool interface as well, by
> clicking "Reply" on the last message in the "Messages" section, at the bottom
of
> the main screen for the CL.
>  
>  
> > 1.       Rename exynos_video_decode_accelerator.h to
> v4l2_video_decode_accelerator.h and .cc as well.
> Yes. 
> > 2.       Introduce v4l2_video_ device.h which defines below interface.
> > Class v4l2_video_device {
> >   V4l2_open(const char *fd,int flags) = 0;
> 
> We don't need any arguments here. Device name and flags should be an
> implementation detail of a concrete class, especially since those arguments
> don't really apply to Tegra for example. Or do you need to pass a device name
to
> your libraries? How do you interact with them?
The device name is really just there to match with v4l2 open call.
I agree these can be implementation detail will move accordingly.

> 
> >   V4l2_close() = 0;
> I think once we open a device, its destructor could take care of closing it.
So
> probably no need for this.
> >  V4l2_ioctl(int flags, void *arg) = 0; //Need ioctl type and argument
> >   // I guess there should be a good way to name below two and are more of
how
> exynos’s polling interrupt mechanism is implemented. On tegra > these are
no-ops
> but since they are called on the “fd” we have to abstract it out here.
> >   V4l2_setdevicepollinterrupt() = 0;
> 
> How do you wake up the polling thread in your case if you need to abort it? 
> 

Since we do not have a real polling fd, we use "StopPoll" api which the library
exposes to abort the poll thread.
Would it be a good idea to rename these set[clear]devicepollinterrupt to rather
more generic to fit into the v4l2_video_device interface ?

> >   V4l2_cleardevicepollinterrupt() = 0;
> I suspect this is not really needed even in EVDA, we could clear immediately
> after acknowledging the interrupt. But this is something for later that I
should
> probably fix myself. 
> >    V4l2_poll() = 0; //Will move all the fdset creation inside exynosV4L2
> device.
> >    // Again these 
> >    V4l2_mmap(void *addr, unsigned int len, int prot, int flags, unsigned int
> offset) = 0;
> >    V4l2_munmap(void *addr, unsigned int len) =0;
> > Private: 
> >   Char device_name_;
> >   Int videodec_fd_;
> >   Int device_poll_interrupt_fd_;
> 
> Do you need all these members for Tegra? I think this should be a pure
abstract
> class.

Yes these can be moved to implemenation class.

> > }
> > The long list of arugments can’t be avoided (except the fd) since we are
> abstracting the video device itself.
> 
> I'm guessing you mean arguments to m{,un}map. Yes, I don't see a better
solution
> for this either right now.
> 
> Also, please drop the V4l2_ prefixes from methods and please follow the
Chromium
> coding style. 
>
System call names will collide with keeping the method names as open(), close()
etc.
What prefix would be better ?
  
> > 3.       Introduce a  file exynos_v4l2_video_device.h and
> exynos_v4l2_video_device.cc which has implementation for Exynos specific
> >V4l2_video_device
> > Class exynos_v4l2_video_device :: public v4l2_video_device {
> > The implementation of v4l2_xxx () will actually make the system calls the
way
> they are implemented now.
>  
> > }
> > 4.       Introduce a file tegra_v4l2_video_device.h and
> tegra_v4l2_video_device.cc. This will have implementations for Tegra specific
> >stuff. It would need the egl_context_ from
> GpuVideoDecodeAccelerator::Initialize.
> 
> This of course should be a separate CL. 

Will first send some CLs to restructure the EVDA and then follow it up by Tegra
addition.

> > And possibly also the library handle since instead of system calls like
> open(), close() we would need to make calls into libtegrav4l2.
> > That should be an implementation detail of the Tegra device class. 
> > Class tegra_v4l2_video_device :: public v4l2_video_device {
> > The implementation of v4l2_xxx() will call into the tegra_v4l2_open()
function
> which the libtegrav4l2 exposes.
> 
> > }
> >5.       In GpuVideoDecodeAccelerator::Initialize() before creating the
> v4l2_video_decode_accelerator object, we would create a >v4l2_video_device and
> pass it as an argument to it. The question is how we instantiate an
appropriate
> one ?
> > Can we keep the video device names different on tegra vs exynos ? So if
> opening of /dev/mfc-dec fails we instantiate the >tegra_v4l2_video_device. Any
> other ideas ?
> > That's fine. You could also use VIDIOC_QUERYCAP to get driver name. I think
> the simplest way is probably to have the factory method try >each of the
classes
> until one of them succeeds in opening their device. I'm actually flirting with
> an idea of making Open static as well and > returning an object of its class
if
> it succeeds in opening the device to the factory method. What do you think?
> > Is the above class hierarchy okay with you ?
>
The real problem is that open() on Tegra will not succeed since we provide a
library api instead of a system call.
I think making static open could be good idea or maybe use open()'s
success/failure to determine this.
If open() succeeds we create the Exynos object else Tegra object.
 
> Yes, looks ok.
>   
> > So basically we will have v4l2_video_decode_accelerator for all the common
> logic on exynos & tegra and use v4l2_video_device to do the > device specific
> implementation.
> 
> That's the idea, yes.
>  
>  
> >Shivdas

Pawel Osciak

6 years, 12 months ago (2013-12-26 07:46:36 UTC) #7

On 2013/12/26 07:03:01, shivdasp wrote:
> > >   // I guess there should be a good way to name below two and are more of
> how
> > exynos’s polling interrupt mechanism is implemented. On tegra > these are
> no-ops
> > but since they are called on the “fd” we have to abstract it out here.
> > >   V4l2_setdevicepollinterrupt() = 0;
> > 
> > How do you wake up the polling thread in your case if you need to abort it? 
> > 
> 
> Since we do not have a real polling fd, we use "StopPoll" api which the
library
> exposes to abort the poll thread.

The poll() call is blocking and these methods are used by EVDA to interrupt it
when something requires us to abort polling the device (like a seek, or
destroy). We need to be able to interrupt a blocking poll() so that we can stop
the polling thread (device_poll_thread_ in EVDA).

I would assume that in your case SetDevicePollInterrupt() should internally call
StopPoll() from your library and signal the sleeping poll() to early return via
some synchronization primitive in your library. But you said those methods are
no-ops in your case?

> Would it be a good idea to rename these set[clear]devicepollinterrupt to
rather
> more generic to fit into the v4l2_video_device interface ?

Could we do away with ClearDevicePollInterrupt() and instead clear it
immediately after being interrupted in poll? I'll let John weigh in on this. So
the interface would be:

Poll()
StopPoll()

> > Also, please drop the V4l2_ prefixes from methods and please follow the
> Chromium
> > coding style. 
> >
> System call names will collide with keeping the method names as open(),
close()
> etc.
> What prefix would be better ?

The simple answer is that once this interface is updated to the Chromium Coding
Style, the method names will have to start with a capital letter.

> > >5.       In GpuVideoDecodeAccelerator::Initialize() before creating the
> > v4l2_video_decode_accelerator object, we would create a >v4l2_video_device
and
> > pass it as an argument to it. The question is how we instantiate an
> appropriate
> > one ?
> > > Can we keep the video device names different on tegra vs exynos ? So if
> > opening of /dev/mfc-dec fails we instantiate the >tegra_v4l2_video_device.
Any
> > other ideas ?
> > > That's fine. You could also use VIDIOC_QUERYCAP to get driver name. I
think
> > the simplest way is probably to have the factory method try >each of the
> classes
> > until one of them succeeds in opening their device. I'm actually flirting
with
> > an idea of making Open static as well and > returning an object of its class
> if
> > it succeeds in opening the device to the factory method. What do you think?
> > > Is the above class hierarchy okay with you ?
> >
> The real problem is that open() on Tegra will not succeed since we provide a
> library api instead of a system call.

I meant above that TVDA::Open() should not succeed if it fails to open the
library.

> I think making static open could be good idea or maybe use open()'s
> success/failure to determine this.
> If open() succeeds we create the Exynos object else Tegra object.
> 

It should not matter which class' method we call, both TVDA and EVDA should be
capable of failing to initialize on a system that doesn't support them and
succeeding on one that does. Also because we may add another implementation in
the future that may need to distinguish between TVDA and itself.

Expand Messages | Collapse Messages