media: Introduce Y8 and Y16 video pixel format (issue 2113243003 by dongseong.hwang@intel.com)

361 views
Skip to first unread message

dongseo...@intel.com

unread,
Jul 1, 2016, 8:19:51 AM7/1/16
to dnic...@chromium.org, dcas...@chromium.org, aleksandar....@intel.com, chromium...@chromium.org, poscia...@chromium.org, feature-me...@chromium.org
Reviewers: dnicoara, Daniele Castagna, aleksandar.stojiljkovic
CL: https://codereview.chromium.org/2113243003/

Message:
dnicoara@, could you review?

It's initial patch for support 8bit and 16bit video stream.

Y8 name is after UVC fourcc, as Y means luma;
https://github.com/torvalds/linux/blob/master/include/uapi/linux/videodev2.h#L489

Depth 16bit stream will use Y16. Y16 can be used for other cases also.
What do you think? specific Z16 is better than unified Y16?
FYI, UVC fourcc has both Z16 and Y16. I think Y16 is enough.

Description:
media: Introduce Y8 and Y16 video pixel format

Depth and infrared camera uses these format.

TODO: native support for depth camera.

BUG=624436

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

Affected files (+20, -3 lines):
M media/base/video_frame.cc
M media/base/video_types.h
M media/base/video_types.cc
M media/renderers/skcanvas_video_renderer.cc


Index: media/base/video_frame.cc
diff --git a/media/base/video_frame.cc b/media/base/video_frame.cc
index 2de6bd36d5cdffb60956aa515a3824671db29b7d..38a6438b5972c16c057e11ea72e017746bb28c88 100644
--- a/media/base/video_frame.cc
+++ b/media/base/video_frame.cc
@@ -125,7 +125,7 @@ bool VideoFrame::IsValidConfig(VideoPixelFormat format,
return true;

// Make sure new formats are properly accounted for in the method.
- static_assert(PIXEL_FORMAT_MAX == 21,
+ static_assert(PIXEL_FORMAT_MAX == 23,
"Added pixel format, please review IsValidConfig()");

if (format == PIXEL_FORMAT_UNKNOWN) {
@@ -535,6 +535,8 @@ size_t VideoFrame::NumPlanes(VideoPixelFormat format) {
case PIXEL_FORMAT_RGB24:
case PIXEL_FORMAT_RGB32:
case PIXEL_FORMAT_MJPEG:
+ case PIXEL_FORMAT_Y8:
+ case PIXEL_FORMAT_Y16:
return 1;
case PIXEL_FORMAT_NV12:
case PIXEL_FORMAT_NV21:
@@ -1041,6 +1043,8 @@ gfx::Size VideoFrame::SampleSize(VideoPixelFormat format, size_t plane) {
case PIXEL_FORMAT_RGB24:
case PIXEL_FORMAT_RGB32:
case PIXEL_FORMAT_MJPEG:
+ case PIXEL_FORMAT_Y8:
+ case PIXEL_FORMAT_Y16:
break;
}
}
@@ -1058,6 +1062,7 @@ int VideoFrame::BytesPerElement(VideoPixelFormat format, size_t plane) {
return 4;
case PIXEL_FORMAT_RGB24:
return 3;
+ case PIXEL_FORMAT_Y16:
case PIXEL_FORMAT_UYVY:
case PIXEL_FORMAT_YUY2:
case PIXEL_FORMAT_YUV420P9:
@@ -1079,6 +1084,7 @@ int VideoFrame::BytesPerElement(VideoPixelFormat format, size_t plane) {
case PIXEL_FORMAT_YV16:
case PIXEL_FORMAT_YV12A:
case PIXEL_FORMAT_YV24:
+ case PIXEL_FORMAT_Y8:
return 1;
case PIXEL_FORMAT_MJPEG:
return 0;
Index: media/base/video_types.cc
diff --git a/media/base/video_types.cc b/media/base/video_types.cc
index f8eeb3e1bb433e03cf390223173204664806611e..9059e87a7d075c9fdd7358cf321e5edf2c5c4f14 100644
--- a/media/base/video_types.cc
+++ b/media/base/video_types.cc
@@ -54,6 +54,10 @@ std::string VideoPixelFormatToString(VideoPixelFormat format) {
return "PIXEL_FORMAT_YUV444P9";
case PIXEL_FORMAT_YUV444P10:
return "PIXEL_FORMAT_YUV444P10";
+ case PIXEL_FORMAT_Y8:
+ return "PIXEL_FORMAT_Y8";
+ case PIXEL_FORMAT_Y16:
+ return "PIXEL_FORMAT_Y16";
}
NOTREACHED() << "Invalid VideoPixelFormat provided: " << format;
return "";
@@ -85,6 +89,8 @@ bool IsYuvPlanar(VideoPixelFormat format) {
case PIXEL_FORMAT_RGB24:
case PIXEL_FORMAT_RGB32:
case PIXEL_FORMAT_MJPEG:
+ case PIXEL_FORMAT_Y8:
+ case PIXEL_FORMAT_Y16:
return false;
}
return false;
@@ -111,6 +117,8 @@ bool IsOpaque(VideoPixelFormat format) {
case PIXEL_FORMAT_YUV422P10:
case PIXEL_FORMAT_YUV444P9:
case PIXEL_FORMAT_YUV444P10:
+ case PIXEL_FORMAT_Y8:
+ case PIXEL_FORMAT_Y16:
return true;
case PIXEL_FORMAT_YV12A:
case PIXEL_FORMAT_ARGB:
Index: media/base/video_types.h
diff --git a/media/base/video_types.h b/media/base/video_types.h
index 7590b1b3a25d4e67a0018eda302f129861aa7cdd..8b225fb3f15db6b4465745b234fcf4963d07a4d4 100644
--- a/media/base/video_types.h
+++ b/media/base/video_types.h
@@ -53,10 +53,11 @@ enum VideoPixelFormat {
PIXEL_FORMAT_YUV422P10 = 19,
PIXEL_FORMAT_YUV444P9 = 20,
PIXEL_FORMAT_YUV444P10 = 21,
-
+ PIXEL_FORMAT_Y8 = 22, // single 8bpp plane.
+ PIXEL_FORMAT_Y16 = 23, // single 16bpp plane.
// Please update UMA histogram enumeration when adding new formats here.
PIXEL_FORMAT_MAX =
- PIXEL_FORMAT_YUV444P10, // Must always be equal to largest entry logged.
+ PIXEL_FORMAT_Y16, // Must always be equal to largest entry logged.
};

// Color space or color range used for the pixels.
Index: media/renderers/skcanvas_video_renderer.cc
diff --git a/media/renderers/skcanvas_video_renderer.cc b/media/renderers/skcanvas_video_renderer.cc
index df59249862b7157b68704f493dd636c0ea7151a5..d5634faab54dab6a792c800dd521f8940426a208 100644
--- a/media/renderers/skcanvas_video_renderer.cc
+++ b/media/renderers/skcanvas_video_renderer.cc
@@ -636,6 +636,8 @@ void SkCanvasVideoRenderer::ConvertVideoFrameToRGBPixels(
case PIXEL_FORMAT_RGB32:
case PIXEL_FORMAT_MJPEG:
case PIXEL_FORMAT_MT21:
+ case PIXEL_FORMAT_Y8:
+ case PIXEL_FORMAT_Y16:
case PIXEL_FORMAT_UNKNOWN:
NOTREACHED();
}


dnic...@chromium.org

unread,
Jul 4, 2016, 10:14:08 AM7/4/16
to dongseo...@intel.com, dcas...@chromium.org, aleksandar....@intel.com, hu...@chromium.org, chromium...@chromium.org, poscia...@chromium.org, feature-me...@chromium.org
+ hubbe@ since he is an owner over some of these files.

I think these are fine, but I'd defer to owners, and kbr@'s reviews since they
have a better view on the requirements.

https://codereview.chromium.org/2113243003/

dongseo...@intel.com

unread,
Jul 4, 2016, 10:55:06 AM7/4/16
to dnic...@chromium.org, dcas...@chromium.org, aleksandar....@intel.com, hu...@chromium.org, k...@chromium.org, chromium...@chromium.org, poscia...@chromium.org, feature-me...@chromium.org
On 2016/07/04 14:14:08, dnicoara wrote:
> + hubbe@ since he is an owner over some of these files.
>
> I think these are fine, but I'd defer to owners, and kbr@'s reviews since they
> have a better view on the requirements.

Thank you for reviewing.

hubbe@, kbr@, could you review?

Y8 will use R8 texture. Y16 will use RG8 texture as RG8 is used in
https://codereview.chromium.org/2122573003/
Rationale to choose RG8, instead of R16
1. GpuMemoryBuffer cannot support R16. (e.g. Mesa supports R8 and GR88 dmabuf)
2. GL_EXT_texture_rg is more widely used than GL_EXT_color_buffer_half_float
(e.g. Mesa v11.2 supports only GL_EXT_texture_rg)

I want to discuss about RG8 and R16 in crbug.com/624436 if you don't agree on
the direction.

https://codereview.chromium.org/2113243003/

fbar...@google.com

unread,
Jul 5, 2016, 2:03:18 PM7/5/16
to dongseo...@intel.com, dnic...@chromium.org, dcas...@chromium.org, aleksandar....@intel.com, hu...@chromium.org, k...@chromium.org, chromium...@chromium.org, poscia...@chromium.org, feature-me...@chromium.org

dongseo...@intel.com

unread,
Jul 6, 2016, 6:30:37 AM7/6/16
to dnic...@chromium.org, dcas...@chromium.org, aleksandar....@intel.com, hu...@chromium.org, k...@chromium.org, fbar...@google.com, chromium...@chromium.org, poscia...@chromium.org, feature-me...@chromium.org
On 2016/07/05 18:03:17, fbarchard1 wrote:
> lgtm

Thank you for reviewing!


hubbe@, kbr@, could you review?

hu...@chromium.org

unread,
Jul 6, 2016, 1:39:01 PM7/6/16
to dongseo...@intel.com, dnic...@chromium.org, dcas...@chromium.org, aleksandar....@intel.com, k...@chromium.org, fbar...@google.com, chromium...@chromium.org, poscia...@chromium.org, feature-me...@chromium.org

https://codereview.chromium.org/2113243003/diff/80001/cc/resources/video_resource_updater.cc
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/2113243003/diff/80001/cc/resources/video_resource_updater.cc#newcode288
cc/resources/video_resource_updater.cc:288: case
media::PIXEL_FORMAT_Y16:
The code below doesn't support bits_per_channel higher than 10.
Thus, this will not work unless you fix the convert-to-half-float code
below.

https://codereview.chromium.org/2113243003/

dongseo...@intel.com

unread,
Jul 6, 2016, 1:45:18 PM7/6/16
to dnic...@chromium.org, dcas...@chromium.org, aleksandar....@intel.com, hu...@chromium.org, k...@chromium.org, fbar...@google.com, chromium...@chromium.org, poscia...@chromium.org, feature-me...@chromium.org
Thank you for reviewing.
On 2016/07/06 17:39:00, hubbe wrote:
> The code below doesn't support bits_per_channel higher than 10.
> Thus, this will not work unless you fix the convert-to-half-float code
below.

k...@chromium.org

unread,
Jul 6, 2016, 2:24:43 PM7/6/16
to dongseo...@intel.com, dnic...@chromium.org, dcas...@chromium.org, aleksandar....@intel.com, hu...@chromium.org, fbar...@google.com, chromium...@chromium.org, poscia...@chromium.org, feature-me...@chromium.org
On 2016/07/06 17:45:18, dshwang wrote:
> On 2016/07/06 17:39:00, hubbe wrote:
> > The code below doesn't support bits_per_channel higher than 10.
> > Thus, this will not work unless you fix the convert-to-half-float
code below.
>
> yes, I know. It will be handled in the next CL;
> https://codereview.chromium.org/2122573003/

Is it wise to land this patch without the next CL? While the other CL is
huge, breaking the code temporarily doesn't seem to me a good path to
take.

https://codereview.chromium.org/2113243003/

dongseo...@intel.com

unread,
Jul 7, 2016, 8:41:08 AM7/7/16
to dnic...@chromium.org, dcas...@chromium.org, aleksandar....@intel.com, hu...@chromium.org, k...@chromium.org, fbar...@google.com, chromium...@chromium.org, poscia...@chromium.org, feature-me...@chromium.org
Thank you for reviewing!
On 2016/07/06 18:24:42, Ken Russell wrote:
> On 2016/07/06 17:45:18, dshwang wrote:
> > On 2016/07/06 17:39:00, hubbe wrote:
> > > The code below doesn't support bits_per_channel higher than 10.
> > > Thus, this will not work unless you fix the convert-to-half-float
code
> below.
> >
> > yes, I know. It will be handled in the next CL;
> > https://codereview.chromium.org/2122573003/
>
> Is it wise to land this patch without the next CL? While the other CL
is huge,
> breaking the code temporarily doesn't seem to me a good path to take.

It doesn't break any code, because any video decoders don't use Y16 yet.
This line is added to avoid build failure.

Next CL has concern about R16 vs. RB8, but this CL will be intact, no
matter what decision happens.

IMO, it's good to land.

https://codereview.chromium.org/2113243003/

hu...@chromium.org

unread,
Jul 7, 2016, 1:51:40 PM7/7/16
to dongseo...@intel.com, dnic...@chromium.org, dcas...@chromium.org, aleksandar....@intel.com, k...@chromium.org, fbar...@google.com, chromium...@chromium.org, poscia...@chromium.org, feature-me...@chromium.org
On 2016/07/07 12:41:08, dshwang wrote:
> On 2016/07/06 18:24:42, Ken Russell wrote:
> > On 2016/07/06 17:45:18, dshwang wrote:
> > > On 2016/07/06 17:39:00, hubbe wrote:
> > > > The code below doesn't support bits_per_channel higher than 10.
> > > > Thus, this will not work unless you fix the
convert-to-half-float code
> > below.
> > >
> > > yes, I know. It will be handled in the next CL;
> > > https://codereview.chromium.org/2122573003/
> >
> > Is it wise to land this patch without the next CL? While the other
CL is huge,
> > breaking the code temporarily doesn't seem to me a good path to
take.
>
> It doesn't break any code, because any video decoders don't use Y16
yet. This
> line is added to avoid build failure.
>
> Next CL has concern about R16 vs. RB8, but this CL will be intact, no
matter
> what decision happens.
>
> IMO, it's good to land.

You need to do the proper error handling.
Always assume that your next CL is going to land a long time from now
and someone else might try to use or change your code in between.

https://codereview.chromium.org/2113243003/

dongseo...@intel.com

unread,
Jul 7, 2016, 2:11:40 PM7/7/16
to dnic...@chromium.org, dcas...@chromium.org, aleksandar....@intel.com, hu...@chromium.org, k...@chromium.org, fbar...@google.com, chromium...@chromium.org, poscia...@chromium.org, feature-me...@chromium.org
Could you review agian? Thank you.
On 2016/07/07 17:51:39, hubbe wrote:
> You need to do the proper error handling.
> Always assume that your next CL is going to land a long time from now
and
> someone else might try to use or change your code in between.

Reply all
Reply to author
Forward
0 new messages