Support external buffer import in VDA interface and add a V4L2SVDA impl.

Support external buffer import in VDA interface and add a V4L2SVDA impl.

Currently, the VDA interface requires implementations to allocate
backing memory for output PictureBuffers. This change introduces a second
mode of operation, the IMPORT mode, which allows the VDA::Client to provide
already allocated buffers for the VDA to import and use.

In order to use it, the Client has to Initialize() the VDA for IMPORT
OutputMode, go through the standard ProvidePictureBuffers(),
AssignPictureBuffers() sequence, but is required to call
ImportBufferForPicture() once for each buffer before it may be used.

VDAs not supporting IMPORT mode will return false from VDA::Initialize().

Also, add support for IMPORT mode to V4L2SVDA and vdaunittest.

This also relands previously reverted crrev.com/1643123003.
The reverted CL was affected by a corner case, which occurred if all of the
following were true: the input stream contained two SPSes in a row, each
causing a resolution change, with no actual frames to decode in between
them, and we happened to hit the second one between calling
ProvidePictureBuffers and before AssignPictureBuffers had a chance to
arrive. This CL fixes that issue by making sure we use the correct
(latest) set of buffers.

Original message:
V4L2SVDA: Move allocation from GPU Child thread to decoder thread.

Allocation may take time, and we don't need to be on the GPU Child
thread to do it. This also removes the explicit synchronization between
the decoder and GPU Child threads during picture set change.

BUG=b/27779397,574241
TEST=vdatest,crosvideo.appspot,seektests

Committed: https://crrev.com/4b267e8cc10a4d98d0295847ecc9e2aa3732d6c5
Cr-Commit-Position: refs/heads/master@{#388168}

[Pawel Osciak, 2016-03-22 02:27:14]

posciak@chromium.org changed reviewers:
+ kcwu@chromium.org, owenlin@chromium.org

[Pawel Osciak, 2016-03-22 02:27:15]

ptal

[kcwu, 2016-03-22 05:42:55]

https://chromiumcodereview.appspot.com/1822983002/diff/1/content/common/gpu/m...
File content/common/gpu/media/tegra_v4l2_device.cc (right):

https://chromiumcodereview.appspot.com/1822983002/diff/1/content/common/gpu/m...
content/common/gpu/media/tegra_v4l2_device.cc:176: int index,
/* index */ and /* type */

https://chromiumcodereview.appspot.com/1822983002/diff/1/content/common/gpu/m...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1822983002/diff/1/content/common/gpu/m...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:525:
output_mode_ = config.output_mode;
Should we validate the value here?

https://chromiumcodereview.appspot.com/1822983002/diff/1/content/common/gpu/m...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1381:
std::vector<EGLImageKHR> egl_images_to_destroy;
unused

https://chromiumcodereview.appspot.com/1822983002/diff/1/content/common/gpu/m...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1574: << "
index=" << buffer_index << " texture_id=" << texture_id;
why not NOTIFY_ERROR and return here?

https://chromiumcodereview.appspot.com/1822983002/diff/1/content/common/gpu/m...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1616: auto
passed_dmabuf_fds(make_scoped_ptr(new std::vector<base::ScopedFD>));
Add "()" for new.
new std::vector<base::ScopedFD>()

https://chromiumcodereview.appspot.com/1822983002/diff/1/content/common/gpu/m...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1618: int fd =
handle.native_pixmap_handle.fd.fd;
I found native_pixelmap_handle is only available if defined(USE_OZONE). Is
defined(USE_OZONE) always true for V4L2SliceVDA?

https://chromiumcodereview.appspot.com/1822983002/diff/1/content/common/gpu/m...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1822983002/diff/1/content/common/gpu/m...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.h:84: bool
at_device;
How about merge |at_device| and |at_client| as single enum to indicate the
ownership and states?
Since 
1. they are not independent (it's not possible true for both at the same time)
2. there is special state now.
v4l2_slice_vda.cc line 1523, it sets output_record.at_client = true but not
really "at client";

https://chromiumcodereview.appspot.com/1822983002/diff/1/media/video/video_de...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1822983002/diff/1/media/video/video_de...
media/video/video_decode_accelerator.h:123: enum class OutputMode {
Move type definition to head of struct.

[kcwu, 2016-03-22 07:47:02]

https://chromiumcodereview.appspot.com/1822983002/diff/1/content/common/gpu/m...
File content/common/gpu/media/generic_v4l2_device.cc (right):

https://chromiumcodereview.appspot.com/1822983002/diff/1/content/common/gpu/m...
content/common/gpu/media/generic_v4l2_device.cc:239: if (num_planes <
dmabuf_fds.size()) {
I don't understand the code what if 1 < dmabuf_fds.size() < num_planes.
Are we assuming (dmabuf_fds.size() == 1 || dmabuf_fds.size() == num_planes) ?

https://chromiumcodereview.appspot.com/1822983002/diff/1/content/common/gpu/m...
File content/common/gpu/media/v4l2_device.h (right):

https://chromiumcodereview.appspot.com/1822983002/diff/1/content/common/gpu/m...
content/common/gpu/media/v4l2_device.h:107: // for which |dmabuf_fds| have been
exported.
Do we need to keep |dmabuf_fds| open after CreateEGLImage() return?

https://chromiumcodereview.appspot.com/1822983002/diff/1/content/common/gpu/m...
content/common/gpu/media/v4l2_device.h:111: EGLContext /* egl_context */,
remove /* */

https://chromiumcodereview.appspot.com/1822983002/diff/1/media/filters/gpu_vi...
File media/filters/gpu_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/1822983002/diff/1/media/filters/gpu_vi...
media/filters/gpu_video_decoder.cc:514: vda_->GetOutputFormat(),
The original code will use XRGB for some cases. But new code seems no ways to
use XRGB.
Is this correct?

[Owen Lin, 2016-03-23 06:32:50]

lgtm % nits

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
File content/common/gpu/media/v4l2_device.h (right):

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.h:107: // for which |dmabuf_fds| have been
exported.
On 2016/03/22 07:47:01, kcwu wrote:
> Do we need to keep |dmabuf_fds| open after CreateEGLImage() return?

How about using std::vector<base::ScopedFD> as the param's type, so it is clear
that CreateEGLImage will take the ownership? If the answer to kcwu's question is
"No".

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:525:
output_mode_ = config.output_mode;
On 2016/03/22 05:42:54, kcwu wrote:
> Should we validate the value here?

Maybe not, we will set this in ArcGVDA. I think ArcGVDA itself is trusted.

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:926:
dqbuf.memory = V4L2_MEMORY_MMAP;
use the ternary operator: "? :"

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1095:
DCHECK_NE(output_record.dmabuf_fds[i].get(), -1);
DCHECK(output_record.dmabuf_fds[i].is_valid());

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1493:
reqbufs.memory = V4L2_MEMORY_MMAP;
ditto.

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1523:
output_record.at_client = true;
As suggested by kcwu, maybe we can make it a single variable, and has the
following different states.
UNBOUND, BOUND, AT_CLIENT, OWNED, AT_HARDWARE....

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1600:
output_record.dmabuf_fds.swap(*passed_dmabuf_fds);
std::move(*passed_dmabuf_fds);

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1603:
DCHECK_EQ(std::count(free_output_buffers_.begin(), free_output_buffers_.end(),
Will it be nice to have a static function:

bool contains(const std::vector<int>& array, int value);

to help to do the check?

[Pawel Osciak, 2016-03-28 01:29:32]

Patchset #2 (id:20001) has been deleted

[Pawel Osciak, 2016-03-28 01:30:26]

Description was changed from

==========
Support external buffer import in VDA interface and add a V4L2SVDA impl.

Currently, the VDA interface requires implementations to allocate
backing memory for output PictureBuffers. This change introduces a second
mode of operation, the IMPORT mode, which allows the VDA::Client to provide
already allocated buffers for the VDA to import and use.

In order to use it, the Client has to Initialize() the VDA for IMPORT
OutputMode, go through the standard ProvidePictureBuffers(),
AssignPictureBuffers() sequence, but is required to call
ImportBufferForPicture() once for each buffer before it may be used.

VDAs not supporting IMPORT mode will return false from VDA::Initialize().

Also, add support for IMPORT mode to V4L2SVDA and vdaunittest.

BUG=b/27779397
TEST=vdatest,crosvideo.appspot
==========

to

==========
Support external buffer import in VDA interface and add a V4L2SVDA impl.

Currently, the VDA interface requires implementations to allocate
backing memory for output PictureBuffers. This change introduces a second
mode of operation, the IMPORT mode, which allows the VDA::Client to provide
already allocated buffers for the VDA to import and use.

In order to use it, the Client has to Initialize() the VDA for IMPORT
OutputMode, go through the standard ProvidePictureBuffers(),
AssignPictureBuffers() sequence, but is required to call
ImportBufferForPicture() once for each buffer before it may be used.

VDAs not supporting IMPORT mode will return false from VDA::Initialize().

Also, add support for IMPORT mode to V4L2SVDA and vdaunittest.

BUG=b/27779397
TEST=vdatest,crosvideo.appspot
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Pawel Osciak, 2016-03-28 01:31:29]

ptal

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/ge...
File content/common/gpu/media/generic_v4l2_device.cc (right):

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/ge...
content/common/gpu/media/generic_v4l2_device.cc:239: if (num_planes <
dmabuf_fds.size()) {
On 2016/03/22 07:47:01, kcwu wrote:
> I don't understand the code what if 1 < dmabuf_fds.size() < num_planes.
> Are we assuming (dmabuf_fds.size() == 1 || dmabuf_fds.size() == num_planes) ?

num_planes is logical planes, while dmabuf_fds.size() is the number of buffers.
It's possible for more than one logical plane to reside in one physical buffer.
At that point we put the remaining planes in the last physical buffer with
nonzero offset. E.g. if for YUV we have num_planes=3, and dmabuf_fds.size()=2,
we put Y in dmabuf[0], U in dmabuf[1] at offset=0 and V in dmabuf[1] at
offset=planesize(U).

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/te...
File content/common/gpu/media/tegra_v4l2_device.cc (right):

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/te...
content/common/gpu/media/tegra_v4l2_device.cc:176: int index,
On 2016/03/22 05:42:54, kcwu wrote:
> /* index */ and /* type */

Done.

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
File content/common/gpu/media/v4l2_device.h (right):

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.h:107: // for which |dmabuf_fds| have been
exported.
On 2016/03/23 06:32:49, Owen Lin wrote:
> On 2016/03/22 07:47:01, kcwu wrote:
> > Do we need to keep |dmabuf_fds| open after CreateEGLImage() return?
> 
> How about using std::vector<base::ScopedFD> as the param's type, so it is
clear
> that CreateEGLImage will take the ownership? If the answer to kcwu's question
is
> "No".

I intended to keep the option of retaining references to the fds for callers
that required it.

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.h:107: // for which |dmabuf_fds| have been
exported.
On 2016/03/22 07:47:01, kcwu wrote:
> Do we need to keep |dmabuf_fds| open after CreateEGLImage() return?

No. Adding to the doc.

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_device.h:111: EGLContext /* egl_context */,
On 2016/03/22 07:47:02, kcwu wrote:
> remove /* */

Done.

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:525:
output_mode_ = config.output_mode;
On 2016/03/22 05:42:54, kcwu wrote:
> Should we validate the value here?

It's an enum class, but I guess new values may be added later on. Done.

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:926:
dqbuf.memory = V4L2_MEMORY_MMAP;
On 2016/03/23 06:32:50, Owen Lin wrote:
> use the ternary operator: "? :"

Done.

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1095:
DCHECK_NE(output_record.dmabuf_fds[i].get(), -1);
On 2016/03/23 06:32:50, Owen Lin wrote:
> DCHECK(output_record.dmabuf_fds[i].is_valid());

Done.

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1381:
std::vector<EGLImageKHR> egl_images_to_destroy;
On 2016/03/22 05:42:54, kcwu wrote:
> unused

Done.

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1493:
reqbufs.memory = V4L2_MEMORY_MMAP;
On 2016/03/23 06:32:50, Owen Lin wrote:
> ditto.

Done.

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1523:
output_record.at_client = true;
On 2016/03/23 06:32:50, Owen Lin wrote:
> As suggested by kcwu, maybe we can make it a single variable, and has the
> following different states.
> UNBOUND, BOUND, AT_CLIENT, OWNED, AT_HARDWARE....

As discussed offline, to be addressed separately.

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1574: << "
index=" << buffer_index << " texture_id=" << texture_id;
On 2016/03/22 05:42:54, kcwu wrote:
> why not NOTIFY_ERROR and return here?

Done.

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1600:
output_record.dmabuf_fds.swap(*passed_dmabuf_fds);
On 2016/03/23 06:32:50, Owen Lin wrote:
> std::move(*passed_dmabuf_fds);

Done.

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1603:
DCHECK_EQ(std::count(free_output_buffers_.begin(), free_output_buffers_.end(),
On 2016/03/23 06:32:50, Owen Lin wrote:
> Will it be nice to have a static function:
> 
> bool contains(const std::vector<int>& array, int value);
> 
> to help to do the check?

Hmm... Sound like a good idea, but I'm on the fence, since this is for DCHECK
only...

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1616: auto
passed_dmabuf_fds(make_scoped_ptr(new std::vector<base::ScopedFD>));
On 2016/03/22 05:42:54, kcwu wrote:
> Add "()" for new.
> new std::vector<base::ScopedFD>()

Done.

https://codereview.chromium.org/1822983002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1618: int fd =
handle.native_pixmap_handle.fd.fd;
On 2016/03/22 05:42:54, kcwu wrote:
> I found native_pixelmap_handle is only available if defined(USE_OZONE). Is
> defined(USE_OZONE) always true for V4L2SliceVDA? 

Done.

https://codereview.chromium.org/1822983002/diff/1/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/1822983002/diff/1/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:514: vda_->GetOutputFormat(),
On 2016/03/22 07:47:02, kcwu wrote:
> The original code will use XRGB for some cases. But new code seems no ways to
> use XRGB.
> Is this correct?

If a VDA doesn't implement GetOutputFormat(), it will default to XRGB returned
by the implementation in the base class.

https://codereview.chromium.org/1822983002/diff/1/media/video/video_decode_ac...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1822983002/diff/1/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:123: enum class OutputMode {
On 2016/03/22 05:42:54, kcwu wrote:
> Move type definition to head of struct.

Done.

[Pawel Osciak, 2016-03-28 01:31:51]

Patchset #2 (id:40001) has been deleted

[Pawel Osciak, 2016-03-28 01:59:10]

posciak@chromium.org changed reviewers:
+ jbauman@chromium.org

[Pawel Osciak, 2016-03-28 01:59:11]

+jbauman for the IsOpaque() change in media/filters/gpu_video_decoder.cc please.

https://codereview.chromium.org/1822983002/diff/1/media/filters/gpu_video_dec...
File media/filters/gpu_video_decoder.cc (right):

https://codereview.chromium.org/1822983002/diff/1/media/filters/gpu_video_dec...
media/filters/gpu_video_decoder.cc:514: vda_->GetOutputFormat(),
On 2016/03/28 01:31:29, Pawel Osciak wrote:
> On 2016/03/22 07:47:02, kcwu wrote:
> > The original code will use XRGB for some cases. But new code seems no ways
to
> > use XRGB.
> > Is this correct?
> 
> If a VDA doesn't implement GetOutputFormat(), it will default to XRGB returned
> by the implementation in the base class.

Sorry, I meant ARGB. As for the choice between XRGB and ARGB, we used to have it
as ARGB only. First of all, the VDA has to report the format it produces, as it
is the only entity that knows about it. Further, I believe I discussed this
issue with original author of the change to allow selection, and I believe we
agreed this was preferred. I'll loop back the original author again for a
confirmation.

[kcwu, 2016-03-28 02:28:36]

https://codereview.chromium.org/1822983002/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/generic_v4l2_device.cc (right):

https://codereview.chromium.org/1822983002/diff/60001/content/common/gpu/medi...
content/common/gpu/media/generic_v4l2_device.cc:272: ++v4l2_plane;
reset plane_offset to 0 here?

[jbauman, 2016-03-28 17:01:49]

On 2016/03/28 01:59:11, Pawel Osciak wrote:
> +jbauman for the IsOpaque() change in media/filters/gpu_video_decoder.cc
please.
> 
>
https://codereview.chromium.org/1822983002/diff/1/media/filters/gpu_video_dec...
> File media/filters/gpu_video_decoder.cc (right):
> 
>
https://codereview.chromium.org/1822983002/diff/1/media/filters/gpu_video_dec...
> media/filters/gpu_video_decoder.cc:514: vda_->GetOutputFormat(),
> On 2016/03/28 01:31:29, Pawel Osciak wrote:
> > On 2016/03/22 07:47:02, kcwu wrote:
> > > The original code will use XRGB for some cases. But new code seems no ways
> to
> > > use XRGB.
> > > Is this correct?
> > 
> > If a VDA doesn't implement GetOutputFormat(), it will default to XRGB
returned
> > by the implementation in the base class.
> 
> Sorry, I meant ARGB. As for the choice between XRGB and ARGB, we used to have
it
> as ARGB only. First of all, the VDA has to report the format it produces, as
it
> is the only entity that knows about it. Further, I believe I discussed this
> issue with original author of the change to allow selection, and I believe we
> agreed this was preferred. I'll loop back the original author again for a
> confirmation.

This should probably always output XRGB instead of ARGB if the video is opaque
(and XRGB would be the correct format to sample it in, e.g. on DXVA).

[Pawel Osciak, 2016-03-31 05:42:33]

ptal

https://chromiumcodereview.appspot.com/1822983002/diff/60001/content/common/g...
File content/common/gpu/media/generic_v4l2_device.cc (right):

https://chromiumcodereview.appspot.com/1822983002/diff/60001/content/common/g...
content/common/gpu/media/generic_v4l2_device.cc:272: ++v4l2_plane;
On 2016/03/28 02:28:36, kcwu wrote:
> reset plane_offset to 0 here?

Good catch, thanks. Done.

[Pawel Osciak, 2016-03-31 05:45:37]

On 2016/03/28 17:01:49, jbauman wrote:
> On 2016/03/28 01:59:11, Pawel Osciak wrote:
> > +jbauman for the IsOpaque() change in media/filters/gpu_video_decoder.cc
> please.
> > 
> >
>
https://codereview.chromium.org/1822983002/diff/1/media/filters/gpu_video_dec...
> > File media/filters/gpu_video_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/1822983002/diff/1/media/filters/gpu_video_dec...
> > media/filters/gpu_video_decoder.cc:514: vda_->GetOutputFormat(),
> > On 2016/03/28 01:31:29, Pawel Osciak wrote:
> > > On 2016/03/22 07:47:02, kcwu wrote:
> > > > The original code will use XRGB for some cases. But new code seems no
ways
> > to
> > > > use XRGB.
> > > > Is this correct?
> > > 
> > > If a VDA doesn't implement GetOutputFormat(), it will default to XRGB
> returned
> > > by the implementation in the base class.
> > 
> > Sorry, I meant ARGB. As for the choice between XRGB and ARGB, we used to
have
> it
> > as ARGB only. First of all, the VDA has to report the format it produces, as
> it
> > is the only entity that knows about it. Further, I believe I discussed this
> > issue with original author of the change to allow selection, and I believe
we
> > agreed this was preferred. I'll loop back the original author again for a
> > confirmation.
> 
> This should probably always output XRGB instead of ARGB if the video is opaque
> (and XRGB would be the correct format to sample it in, e.g. on DXVA).

Modified to return PIXEL_FORMAT_UNKNOWN so that the client can attempt to make a
decision on its own. PTAL.

[Pawel Osciak, 2016-03-31 06:07:32]

On 2016/03/31 05:45:37, Pawel Osciak wrote:
> On 2016/03/28 17:01:49, jbauman wrote:
> > On 2016/03/28 01:59:11, Pawel Osciak wrote:
> > > +jbauman for the IsOpaque() change in media/filters/gpu_video_decoder.cc
> > please.
> > > 
> > >
> >
>
https://codereview.chromium.org/1822983002/diff/1/media/filters/gpu_video_dec...
> > > File media/filters/gpu_video_decoder.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1822983002/diff/1/media/filters/gpu_video_dec...
> > > media/filters/gpu_video_decoder.cc:514: vda_->GetOutputFormat(),
> > > On 2016/03/28 01:31:29, Pawel Osciak wrote:
> > > > On 2016/03/22 07:47:02, kcwu wrote:
> > > > > The original code will use XRGB for some cases. But new code seems no
> ways
> > > to
> > > > > use XRGB.
> > > > > Is this correct?
> > > > 
> > > > If a VDA doesn't implement GetOutputFormat(), it will default to XRGB
> > returned
> > > > by the implementation in the base class.
> > > 
> > > Sorry, I meant ARGB. As for the choice between XRGB and ARGB, we used to
> have
> > it
> > > as ARGB only. First of all, the VDA has to report the format it produces,
as
> > it
> > > is the only entity that knows about it. Further, I believe I discussed
this
> > > issue with original author of the change to allow selection, and I believe
> we
> > > agreed this was preferred. I'll loop back the original author again for a
> > > confirmation.
> > 
> > This should probably always output XRGB instead of ARGB if the video is
opaque
> > (and XRGB would be the correct format to sample it in, e.g. on DXVA).
> 
> Modified to return PIXEL_FORMAT_UNKNOWN so that the client can attempt to make
a
> decision on its own. PTAL.

Ideally I'd love to be able to pass UNKNOWN all the way up in the VideoFrame and
let the underlying platform code/drivers figure it out as they do right now in
such case. Unfortunately, I don't think we can easily do that.

[Pawel Osciak, 2016-03-31 06:09:20]

On 2016/03/31 06:07:32, Pawel Osciak wrote:
> On 2016/03/31 05:45:37, Pawel Osciak wrote:
> > On 2016/03/28 17:01:49, jbauman wrote:
> > > On 2016/03/28 01:59:11, Pawel Osciak wrote:
> > > > +jbauman for the IsOpaque() change in media/filters/gpu_video_decoder.cc
> > > please.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1822983002/diff/1/media/filters/gpu_video_dec...
> > > > File media/filters/gpu_video_decoder.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1822983002/diff/1/media/filters/gpu_video_dec...
> > > > media/filters/gpu_video_decoder.cc:514: vda_->GetOutputFormat(),
> > > > On 2016/03/28 01:31:29, Pawel Osciak wrote:
> > > > > On 2016/03/22 07:47:02, kcwu wrote:
> > > > > > The original code will use XRGB for some cases. But new code seems
no
> > ways
> > > > to
> > > > > > use XRGB.
> > > > > > Is this correct?
> > > > > 
> > > > > If a VDA doesn't implement GetOutputFormat(), it will default to XRGB
> > > returned
> > > > > by the implementation in the base class.
> > > > 
> > > > Sorry, I meant ARGB. As for the choice between XRGB and ARGB, we used to
> > have
> > > it
> > > > as ARGB only. First of all, the VDA has to report the format it
produces,
> as
> > > it
> > > > is the only entity that knows about it. Further, I believe I discussed
> this
> > > > issue with original author of the change to allow selection, and I
believe
> > we
> > > > agreed this was preferred. I'll loop back the original author again for
a
> > > > confirmation.
> > > 
> > > This should probably always output XRGB instead of ARGB if the video is
> opaque
> > > (and XRGB would be the correct format to sample it in, e.g. on DXVA).
> > 
> > Modified to return PIXEL_FORMAT_UNKNOWN so that the client can attempt to
make
> a
> > decision on its own. PTAL.
> 
> Ideally I'd love to be able to pass UNKNOWN all the way up in the VideoFrame
and
> let the underlying platform code/drivers figure it out as they do right now in
> such case. Unfortunately, I don't think we can easily do that.

On second thought, really ideally we should probably be passing the right format
always, so adding implementations of GetOutputFormat() to all VDAs.

[kcwu, 2016-03-31 07:33:43]

Adding GetOutputFormat() to all VDAs sounds good to me. I will leave it up to
you. Besides output format, lgtm.

[Pawel Osciak, 2016-03-31 07:36:36]

On 2016/03/31 07:33:43, kcwu wrote:
> Adding GetOutputFormat() to all VDAs sounds good to me. I will leave it up to
> you. Besides output format, lgtm.

This will have to be done in followup CLs, I'm not familiar enough with all
details of each platform.

[jbauman, 2016-04-01 01:52:14]

lgtm

[kcwu, 2016-04-06 09:05:03]

lgtm

[Pawel Osciak, 2016-04-13 06:15:46]

Patchset #5 (id:120001) has been deleted

[Pawel Osciak, 2016-04-13 06:25:37]

Patchset #6 (id:160001) has been deleted

[Pawel Osciak, 2016-04-13 06:26:33]

Patchset #5 (id:140001) has been deleted

[Pawel Osciak, 2016-04-13 06:26:50]

ptal

[Pawel Osciak, 2016-04-13 06:28:14]

Description was changed from

==========
Support external buffer import in VDA interface and add a V4L2SVDA impl.

Currently, the VDA interface requires implementations to allocate
backing memory for output PictureBuffers. This change introduces a second
mode of operation, the IMPORT mode, which allows the VDA::Client to provide
already allocated buffers for the VDA to import and use.

In order to use it, the Client has to Initialize() the VDA for IMPORT
OutputMode, go through the standard ProvidePictureBuffers(),
AssignPictureBuffers() sequence, but is required to call
ImportBufferForPicture() once for each buffer before it may be used.

VDAs not supporting IMPORT mode will return false from VDA::Initialize().

Also, add support for IMPORT mode to V4L2SVDA and vdaunittest.

BUG=b/27779397
TEST=vdatest,crosvideo.appspot
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Support external buffer import in VDA interface and add a V4L2SVDA impl.

Currently, the VDA interface requires implementations to allocate
backing memory for output PictureBuffers. This change introduces a second
mode of operation, the IMPORT mode, which allows the VDA::Client to provide
already allocated buffers for the VDA to import and use.

In order to use it, the Client has to Initialize() the VDA for IMPORT
OutputMode, go through the standard ProvidePictureBuffers(),
AssignPictureBuffers() sequence, but is required to call
ImportBufferForPicture() once for each buffer before it may be used.

VDAs not supporting IMPORT mode will return false from VDA::Initialize().

Also, add support for IMPORT mode to V4L2SVDA and vdaunittest.

This also relands previously reverted crrev.com/1643123003.
The reverted CL was affected by a corner case, which occurred if all of the
following were true: the input stream contained two SPSes in a row, each
causing a resolution change, with no actual frames to decode in between
them, and we happened to hit the second one between calling
ProvidePictureBuffers and before AssignPictureBuffers had a chance to
arrive. This CL fixes that issue by making sure we use the correct
(latest) set of buffers.

Original message:
V4L2SVDA: Move allocation from GPU Child thread to decoder thread.

Allocation may take time, and we don't need to be on the GPU Child
thread to do it. This also removes the explicit synchronization between
the decoder and GPU Child threads during picture set change.

BUG=b/27779397,574241
TEST=vdatest,crosvideo.appspot,seektests
==========

[kcwu, 2016-04-13 08:05:43]

I skip the code from reland of crrev.com/1643123003. Besides that, still lgtm.

[wuchengli, 2016-04-13 10:54:24]

wuchengli@chromium.org changed reviewers:
+ wuchengli@chromium.org

[wuchengli, 2016-04-13 10:54:25]

LGTM for the parts of reland of crrev.com/1643123003

https://codereview.chromium.org/1822983002/diff/180001/content/common/gpu/med...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1822983002/diff/180001/content/common/gpu/med...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:764: 
Please document what the state is before setting it to kAwaitingPictureBuffers.
Also we set the state_ to kAwaitingPictureBuffers because we don't want to
continue decoding.

[Pawel Osciak, 2016-04-14 00:53:04]

posciak@chromium.org changed reviewers:
+ jam@chromium.org, sandersd@chromium.org

[Pawel Osciak, 2016-04-14 00:53:05]

sandersd@chromium.org: Please OWNERS for media

jam@chromium.org: Please OWNERS for content

Thank you.

[jam, 2016-04-14 20:40:27]

On 2016/04/14 00:53:05, Pawel Osciak wrote:
> mailto:sandersd@chromium.org: Please OWNERS for media
> 
> mailto:jam@chromium.org: Please OWNERS for content
> 
> Thank you.

content/public lgtm 

please get content/renderer/media/ owner to review these files.

[Pawel Osciak, 2016-04-15 00:18:17]

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

[Pawel Osciak, 2016-04-15 00:19:04]

+mcasas@ for content/renderer/media please. Thank you.

[sandersd (OOO until July 31), 2016-04-15 00:19:52]

lgtm

[mcasas, 2016-04-16 01:53:05]

On 2016/04/15 00:19:52, sandersd wrote:
> lgtm

content/renderer/media RS LGTM

[Pawel Osciak, 2016-04-18 06:17:32]

https://chromiumcodereview.appspot.com/1822983002/diff/180001/content/common/...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1822983002/diff/180001/content/common/...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:764: 
On 2016/04/13 10:54:25, wuchengli wrote:
> Please document what the state is before setting it to
kAwaitingPictureBuffers.
> Also we set the state_ to kAwaitingPictureBuffers because we don't want to
> continue decoding.

Done.

[Pawel Osciak, 2016-04-18 06:17:38]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2016-04-18 06:17:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
owenlin@chromium.org, jbauman@chromium.org, mcasas@chromium.org,
sandersd@chromium.org, jam@chromium.org, kcwu@chromium.org,
wuchengli@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1822983002/#ps200001 (title: " ")

[commit-bot: I haz the power, 2016-04-18 06:17:51]

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

[commit-bot: I haz the power, 2016-04-18 06:56:11]

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

[commit-bot: I haz the power, 2016-04-18 06:56:13]

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

[Pawel Osciak, 2016-04-18 09:34:53]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2016-04-18 09:34:53]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
owenlin@chromium.org, sandersd@chromium.org, jam@chromium.org,
kcwu@chromium.org, wuchengli@chromium.org, jbauman@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1822983002/#ps220001 (title: " ")

[commit-bot: I haz the power, 2016-04-18 09:35:09]

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

[commit-bot: I haz the power, 2016-04-18 10:18:39]

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

[commit-bot: I haz the power, 2016-04-18 10:18:41]

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

[Pawel Osciak, 2016-04-19 06:08:28]

Patchset #7 (id:220001) has been deleted

[Owen Lin, 2016-04-19 06:55:14]

vda_unitttest lgtm

[Pawel Osciak, 2016-04-19 07:01:52]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2016-04-19 07:01:52]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
sandersd@chromium.org, jam@chromium.org, kcwu@chromium.org,
wuchengli@chromium.org, jbauman@chromium.org, owenlin@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1822983002/#ps260001 (title: " ")

[commit-bot: I haz the power, 2016-04-19 07:02:14]

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

[commit-bot: I haz the power, 2016-04-19 08:40:25]

Description was changed from

==========
Support external buffer import in VDA interface and add a V4L2SVDA impl.

Currently, the VDA interface requires implementations to allocate
backing memory for output PictureBuffers. This change introduces a second
mode of operation, the IMPORT mode, which allows the VDA::Client to provide
already allocated buffers for the VDA to import and use.

In order to use it, the Client has to Initialize() the VDA for IMPORT
OutputMode, go through the standard ProvidePictureBuffers(),
AssignPictureBuffers() sequence, but is required to call
ImportBufferForPicture() once for each buffer before it may be used.

VDAs not supporting IMPORT mode will return false from VDA::Initialize().

Also, add support for IMPORT mode to V4L2SVDA and vdaunittest.

This also relands previously reverted crrev.com/1643123003.
The reverted CL was affected by a corner case, which occurred if all of the
following were true: the input stream contained two SPSes in a row, each
causing a resolution change, with no actual frames to decode in between
them, and we happened to hit the second one between calling
ProvidePictureBuffers and before AssignPictureBuffers had a chance to
arrive. This CL fixes that issue by making sure we use the correct
(latest) set of buffers.

Original message:
V4L2SVDA: Move allocation from GPU Child thread to decoder thread.

Allocation may take time, and we don't need to be on the GPU Child
thread to do it. This also removes the explicit synchronization between
the decoder and GPU Child threads during picture set change.

BUG=b/27779397,574241
TEST=vdatest,crosvideo.appspot,seektests
==========

to

==========
Support external buffer import in VDA interface and add a V4L2SVDA impl.

Currently, the VDA interface requires implementations to allocate
backing memory for output PictureBuffers. This change introduces a second
mode of operation, the IMPORT mode, which allows the VDA::Client to provide
already allocated buffers for the VDA to import and use.

In order to use it, the Client has to Initialize() the VDA for IMPORT
OutputMode, go through the standard ProvidePictureBuffers(),
AssignPictureBuffers() sequence, but is required to call
ImportBufferForPicture() once for each buffer before it may be used.

VDAs not supporting IMPORT mode will return false from VDA::Initialize().

Also, add support for IMPORT mode to V4L2SVDA and vdaunittest.

This also relands previously reverted crrev.com/1643123003.
The reverted CL was affected by a corner case, which occurred if all of the
following were true: the input stream contained two SPSes in a row, each
causing a resolution change, with no actual frames to decode in between
them, and we happened to hit the second one between calling
ProvidePictureBuffers and before AssignPictureBuffers had a chance to
arrive. This CL fixes that issue by making sure we use the correct
(latest) set of buffers.

Original message:
V4L2SVDA: Move allocation from GPU Child thread to decoder thread.

Allocation may take time, and we don't need to be on the GPU Child
thread to do it. This also removes the explicit synchronization between
the decoder and GPU Child threads during picture set change.

BUG=b/27779397,574241
TEST=vdatest,crosvideo.appspot,seektests
==========

[commit-bot: I haz the power, 2016-04-19 08:40:27]

Committed patchset #8 (id:260001)

[commit-bot: I haz the power, 2016-04-19 08:41:44]

Description was changed from

==========
Support external buffer import in VDA interface and add a V4L2SVDA impl.

Currently, the VDA interface requires implementations to allocate
backing memory for output PictureBuffers. This change introduces a second
mode of operation, the IMPORT mode, which allows the VDA::Client to provide
already allocated buffers for the VDA to import and use.

In order to use it, the Client has to Initialize() the VDA for IMPORT
OutputMode, go through the standard ProvidePictureBuffers(),
AssignPictureBuffers() sequence, but is required to call
ImportBufferForPicture() once for each buffer before it may be used.

VDAs not supporting IMPORT mode will return false from VDA::Initialize().

Also, add support for IMPORT mode to V4L2SVDA and vdaunittest.

This also relands previously reverted crrev.com/1643123003.
The reverted CL was affected by a corner case, which occurred if all of the
following were true: the input stream contained two SPSes in a row, each
causing a resolution change, with no actual frames to decode in between
them, and we happened to hit the second one between calling
ProvidePictureBuffers and before AssignPictureBuffers had a chance to
arrive. This CL fixes that issue by making sure we use the correct
(latest) set of buffers.

Original message:
V4L2SVDA: Move allocation from GPU Child thread to decoder thread.

Allocation may take time, and we don't need to be on the GPU Child
thread to do it. This also removes the explicit synchronization between
the decoder and GPU Child threads during picture set change.

BUG=b/27779397,574241
TEST=vdatest,crosvideo.appspot,seektests
==========

to

==========
Support external buffer import in VDA interface and add a V4L2SVDA impl.

Currently, the VDA interface requires implementations to allocate
backing memory for output PictureBuffers. This change introduces a second
mode of operation, the IMPORT mode, which allows the VDA::Client to provide
already allocated buffers for the VDA to import and use.

In order to use it, the Client has to Initialize() the VDA for IMPORT
OutputMode, go through the standard ProvidePictureBuffers(),
AssignPictureBuffers() sequence, but is required to call
ImportBufferForPicture() once for each buffer before it may be used.

VDAs not supporting IMPORT mode will return false from VDA::Initialize().

Also, add support for IMPORT mode to V4L2SVDA and vdaunittest.

This also relands previously reverted crrev.com/1643123003.
The reverted CL was affected by a corner case, which occurred if all of the
following were true: the input stream contained two SPSes in a row, each
causing a resolution change, with no actual frames to decode in between
them, and we happened to hit the second one between calling
ProvidePictureBuffers and before AssignPictureBuffers had a chance to
arrive. This CL fixes that issue by making sure we use the correct
(latest) set of buffers.

Original message:
V4L2SVDA: Move allocation from GPU Child thread to decoder thread.

Allocation may take time, and we don't need to be on the GPU Child
thread to do it. This also removes the explicit synchronization between
the decoder and GPU Child threads during picture set change.

BUG=b/27779397,574241
TEST=vdatest,crosvideo.appspot,seektests

Committed: https://crrev.com/4b267e8cc10a4d98d0295847ecc9e2aa3732d6c5
Cr-Commit-Position: refs/heads/master@{#388168}
==========

[commit-bot: I haz the power, 2016-04-19 08:41:46]

Patchset 8 (id:??) landed as
https://crrev.com/4b267e8cc10a4d98d0295847ecc9e2aa3732d6c5
Cr-Commit-Position: refs/heads/master@{#388168}

[victorhsieh0, 2016-04-20 20:33:06]

victorhsieh@google.com changed reviewers:
+ victorhsieh@google.com

[victorhsieh0, 2016-04-20 20:33:07]

This change has broken arm-generic-chromium-pfq and nyan-chrome-pfq due to
unused variable when USE_OZONE is undefined.

https://uberchromegw.corp.google.com/i/chromeos/builders/arm-generic-chromium...
https://uberchromegw.corp.google.com/i/chromeos/builders/nyan-chrome-pfq/buil...

[victorhsieh0, 2016-04-20 20:52:59]

Hopefully this should fix the build.
https://codereview.chromium.org/1907593003/