Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2370453003:
12-bit vp9 video support (Closed)
|
Created:
4 years, 3 months ago by hubbe Modified:
4 years, 2 months ago CC:
Aaron Boodman, abarth-chromium, alokp+watch_chromium.org, cc-bugs_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, Johann, posciak+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for 12 bit 420, 422 and 444 YUV video frames. Make sure that we can detect then properly when they come out of libvpx and that we can convert them to half-floats properly when we send them to the compositor.
BUG=445071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/6cf2c09cb064b8530a1ac9dfa79a946410973d83
Cr-Commit-Position: refs/heads/master@{#421610}
Patch Set 1 #
Total comments: 4
Patch Set 2 : avoid fall-through #Patch Set 3 : vpx doesn't support 9-bit + magic float->half-float function #Patch Set 4 : typO #Patch Set 5 : half-float conversion accuracy test added #Patch Set 6 : merged #
Total comments: 12
Patch Set 7 : comments addressed + comment added #
Total comments: 10
Patch Set 8 : comments addressed #Patch Set 9 : build fix #
Total comments: 13
Patch Set 10 : comments addressed + histograms.xml fix #
Total comments: 4
Patch Set 11 : typO #Messages
Total messages: 69 (43 generated)
Description was changed from ========== 12-bit video support BUG=445071 ========== to ========== 12-bit video support BUG=445071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hubbe@chromium.org changed reviewers: + dalecurtis@chromium.org, danakj@chromium.org
https://codereview.chromium.org/2370453003/diff/1/media/renderers/skcanvas_vi... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2370453003/diff/1/media/renderers/skcanvas_vi... media/renderers/skcanvas_video_renderer.cc:457: shift++; Why the change?
https://codereview.chromium.org/2370453003/diff/1/media/renderers/skcanvas_vi... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2370453003/diff/1/media/renderers/skcanvas_vi... media/renderers/skcanvas_video_renderer.cc:457: shift++; On 2016/09/23 23:42:24, DaleCurtis wrote: > Why the change? Because of fall-through?
https://codereview.chromium.org/2370453003/diff/1/media/renderers/skcanvas_vi... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2370453003/diff/1/media/renderers/skcanvas_vi... media/renderers/skcanvas_video_renderer.cc:457: shift++; On 2016/09/23 at 23:43:06, hubbe wrote: > On 2016/09/23 23:42:24, DaleCurtis wrote: > > Why the change? > > Because of fall-through? Oooh, yuck. This seems like it would be way more legible w/ a break and exact assignment instead of fall-through + accumulation?
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2370453003/diff/1/media/renderers/skcanvas_vi... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2370453003/diff/1/media/renderers/skcanvas_vi... media/renderers/skcanvas_video_renderer.cc:457: shift++; On 2016/09/23 23:45:42, DaleCurtis wrote: > On 2016/09/23 at 23:43:06, hubbe wrote: > > On 2016/09/23 23:42:24, DaleCurtis wrote: > > > Why the change? > > > > Because of fall-through? > > Oooh, yuck. This seems like it would be way more legible w/ a break and exact > assignment instead of fall-through + accumulation? It's longer and seems a bit more error-prone to me, but ok.
media/ lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater.cc:510: // Note that the current method of converting integers to half-floats What about this comment? https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater.cc:573: bits_per_channel <= 10) { what if bits > 10? https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater.h:84: static void MakeHalfFloats(const uint16_t* src, There are 4 parameters, all are shortened names, and there's 0 comments explaining what this function does or what the parameters are. Also, maybe this shouldn't be on VideoResourceUpdater? Though I'd have to reverse engineer some documentation for this function to suggest anywhere else. https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... File cc/resources/video_resource_updater_unittest.cc (right): https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:507: namespace { whitespace below this https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:508: double from_half_float(uint16_t f) { FromHalfFloat Please a better name than |f|, and a comment explaining what this function does.
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
All comments addressed. Also added a comment that explains how MakeHalfFloats work. https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater.cc:510: // Note that the current method of converting integers to half-floats On 2016/09/26 20:35:04, danakj wrote: > What about this comment? Removed. https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater.cc:573: bits_per_channel <= 10) { On 2016/09/26 20:35:04, danakj wrote: > what if bits > 10? Then offset and multiplier should have their default values, because values has been converted to a half-float between 0.0 and 1.0. Frank is looking in to making and SSE2 version of the float-to-half-float code, and if that's fast enough we can get rid of the OR trick and the offset/multiplier. Of course, all of this is moot if we end up using RG88 textures to represent 16-bit values, but we haven't done enough experimentation to see if that is the best option yet. https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater.h:84: static void MakeHalfFloats(const uint16_t* src, On 2016/09/26 20:35:04, danakj wrote: > There are 4 parameters, all are shortened names, and there's 0 comments > explaining what this function does or what the parameters are. > > Also, maybe this shouldn't be on VideoResourceUpdater? Though I'd have to > reverse engineer some documentation for this function to suggest anywhere else. Comment added. https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... File cc/resources/video_resource_updater_unittest.cc (right): https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:507: namespace { On 2016/09/26 20:35:04, danakj wrote: > whitespace below this Done. https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:508: double from_half_float(uint16_t f) { On 2016/09/26 20:35:04, danakj wrote: > FromHalfFloat > > Please a better name than |f|, and a comment explaining what this function does. Done.
https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater.cc:573: bits_per_channel <= 10) { On 2016/09/26 21:19:36, hubbe wrote: > On 2016/09/26 20:35:04, danakj wrote: > > what if bits > 10? > > Then offset and multiplier should have their default values, because values has > been converted to a half-float between 0.0 and 1.0. > > Frank is looking in to making and SSE2 version of the float-to-half-float code, > and if that's fast enough we can get rid of the OR trick and the > offset/multiplier. > > Of course, all of this is moot if we end up using RG88 textures to represent > 16-bit values, but we haven't done enough experimentation to see if that is the > best option yet. Can you explain in a comment too? https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.cc:296: void VideoResourceUpdater::MakeHalfFloats(const uint16_t* src, Maybe MultiplyHalfFloats would be a more accurate/descriptive name? https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.cc:304: // is he difference in exponent bias between 32-bit and is the https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... File cc/resources/video_resource_updater_unittest.cc (right): https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:536: float mult = 1.0f / (num_values - 1); Why this mult? https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:542: FromHalfFloat(out[i] + 1) - FromHalfFloat(out[i])) What does +1 on a half-float do to it? Can you leave a comment
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2370453003/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater.cc:573: bits_per_channel <= 10) { On 2016/09/26 22:10:39, danakj wrote: > On 2016/09/26 21:19:36, hubbe wrote: > > On 2016/09/26 20:35:04, danakj wrote: > > > what if bits > 10? > > > > Then offset and multiplier should have their default values, because values > has > > been converted to a half-float between 0.0 and 1.0. > > > > Frank is looking in to making and SSE2 version of the float-to-half-float > code, > > and if that's fast enough we can get rid of the OR trick and the > > offset/multiplier. > > > > Of course, all of this is moot if we end up using RG88 textures to represent > > 16-bit values, but we haven't done enough experimentation to see if that is > the > > best option yet. > > Can you explain in a comment too? Done. https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.cc:296: void VideoResourceUpdater::MakeHalfFloats(const uint16_t* src, On 2016/09/26 22:10:39, danakj wrote: > Maybe MultiplyHalfFloats would be a more accurate/descriptive name? I think I did a terrible job of explaining what this function does. Trying again.... https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.cc:304: // is he difference in exponent bias between 32-bit and On 2016/09/26 22:10:39, danakj wrote: > is the Done. https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... File cc/resources/video_resource_updater_unittest.cc (right): https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:536: float mult = 1.0f / (num_values - 1); On 2016/09/26 22:10:39, danakj wrote: > Why this mult? Is "multiplier" better? Or did I misunderstand the question? https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:542: FromHalfFloat(out[i] + 1) - FromHalfFloat(out[i])) On 2016/09/26 22:10:39, danakj wrote: > What does +1 on a half-float do to it? Can you leave a comment Done.
https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... File cc/resources/video_resource_updater_unittest.cc (right): https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:536: float mult = 1.0f / (num_values - 1); On 2016/09/27 00:03:25, hubbe wrote: > On 2016/09/26 22:10:39, danakj wrote: > > Why this mult? > > Is "multiplier" better? Or did I misunderstand the question? I was wondering why 1/(n-1)?
https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... File cc/resources/video_resource_updater_unittest.cc (right): https://codereview.chromium.org/2370453003/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:536: float mult = 1.0f / (num_values - 1); On 2016/09/27 00:15:58, danakj wrote: > On 2016/09/27 00:03:25, hubbe wrote: > > On 2016/09/26 22:10:39, danakj wrote: > > > Why this mult? > > > > Is "multiplier" better? Or did I misunderstand the question? > > I was wondering why 1/(n-1)? Ah. I did add a comment for that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for your changes things are reading much more clearly IMO. cc LGTM with a few more comments/nits. https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater.cc:576: if (plane_resource.resource_format() == LUMINANCE_F16 && One more thought, we're doing this if thing twice essentially, which means they need to not diverge. Above we have also checked the same things. Would it be more clear if we set a bool use_multiplier_for_half_float_output = false up above or something, and set it to true at the place where we do the OR path? https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater.h:84: // |src| is an array of integers in range 0 .. (1<<bits_per_channel) - 1 nit: 0..2^{bits_per_channel}-1 ? or maybe thats my latex background kicking in. https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... File cc/resources/video_resource_updater_unittest.cc (right): https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:529: unsigned short in[1 << 12]; can we name these 2 vars to tell their context a bit more? like in => integers? out => half_floats? That would make the EXPECT_NEAR more clear I think. https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:539: float multiplier = 1.0f / (num_values - 1); nit: double (and 1.0 not 1.f) since you're returning double FromHalfFloat? https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:542: // We expect the result to be within +/- one LSB. nit: can you spell out LSB https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:546: float expected_precision = Thanks this is helpful
> 12-bit video support Can you write more in the CL description about this? It's a pretty big CL to describe with just that.
Description was changed from ========== 12-bit video support BUG=445071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Add support for 12 bit 420, 422 and 444 YUV video frames. Make sure that we can detect then properly when they come out of libvpx and that we can convert them to half-floats properly when we send them to the compositor. BUG=445071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater.cc:576: if (plane_resource.resource_format() == LUMINANCE_F16 && On 2016/09/27 20:49:11, danakj wrote: > One more thought, we're doing this if thing twice essentially, which means they > need to not diverge. Above we have also checked the same things. Would it be > more clear if we set a bool use_multiplier_for_half_float_output = false up > above or something, and set it to true at the place where we do the OR path? I don't really want to set a bool in every row, but how about I put it above the loops where we do some similar if statements? https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater.h:84: // |src| is an array of integers in range 0 .. (1<<bits_per_channel) - 1 On 2016/09/27 20:49:11, danakj wrote: > nit: 0..2^{bits_per_channel}-1 ? or maybe thats my latex background kicking in. 1 << x reads exactly like 2^x to me, but that might be caused by accumulated C-syntax brain injuries. :) Done. https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... File cc/resources/video_resource_updater_unittest.cc (right): https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:529: unsigned short in[1 << 12]; On 2016/09/27 20:49:11, danakj wrote: > can we name these 2 vars to tell their context a bit more? like > > in => integers? > out => half_floats? > > That would make the EXPECT_NEAR more clear I think. Done. https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:539: float multiplier = 1.0f / (num_values - 1); On 2016/09/27 20:49:11, danakj wrote: > nit: double (and 1.0 not 1.f) since you're returning double FromHalfFloat? Done. https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:542: // We expect the result to be within +/- one LSB. On 2016/09/27 20:49:11, danakj wrote: > nit: can you spell out LSB Done. https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater_unittest.cc:546: float expected_precision = On 2016/09/27 20:49:11, danakj wrote: > Thanks this is helpful Done.
https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2370453003/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater.cc:576: if (plane_resource.resource_format() == LUMINANCE_F16 && On 2016/09/27 22:27:36, hubbe wrote: > On 2016/09/27 20:49:11, danakj wrote: > > One more thought, we're doing this if thing twice essentially, which means > they > > need to not diverge. Above we have also checked the same things. Would it be > > more clear if we set a bool use_multiplier_for_half_float_output = false up > > above or something, and set it to true at the place where we do the OR path? > > I don't really want to set a bool in every row, but how about I put it above the > loops where we do some similar if statements? That's cool too if you agree it's more clear
hubbe@chromium.org changed reviewers: + rkaplow@chromium.org, tsepez@chromium.org
+tsepez for mojo files +rkaplow for histograms.xml
mojo LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm lg % typo in xml https://codereview.chromium.org/2370453003/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2370453003/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:99512: + <int value="22" label="YUV420P129"/> think this 9 is a typo
https://codereview.chromium.org/2370453003/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2370453003/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:99512: + <int value="22" label="YUV420P129"/> On 2016/09/28 14:13:40, rkaplow wrote: > think this 9 is a typo Oops, fixed.
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, danakj@chromium.org, tsepez@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2370453003/#ps200001 (title: "typO")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add support for 12 bit 420, 422 and 444 YUV video frames. Make sure that we can detect then properly when they come out of libvpx and that we can convert them to half-floats properly when we send them to the compositor. BUG=445071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Add support for 12 bit 420, 422 and 444 YUV video frames. Make sure that we can detect then properly when they come out of libvpx and that we can convert them to half-floats properly when we send them to the compositor. BUG=445071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Add support for 12 bit 420, 422 and 444 YUV video frames. Make sure that we can detect then properly when they come out of libvpx and that we can convert them to half-floats properly when we send them to the compositor. BUG=445071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Add support for 12 bit 420, 422 and 444 YUV video frames. Make sure that we can detect then properly when they come out of libvpx and that we can convert them to half-floats properly when we send them to the compositor. BUG=445071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/6cf2c09cb064b8530a1ac9dfa79a946410973d83 Cr-Commit-Position: refs/heads/master@{#421610} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/6cf2c09cb064b8530a1ac9dfa79a946410973d83 Cr-Commit-Position: refs/heads/master@{#421610}
Message was sent while issue was closed.
hubbe@, great. How did you produce bear-320x180-hi12p-vp9.webm ?
Message was sent while issue was closed.
On 2016/09/29 11:27:14, dshwang wrote: > hubbe@, great. How did you produce bear-320x180-hi12p-vp9.webm ? /usr/local/hi10p/bin/ffmpeg -i bear-1280x720.mp4 -sws_flags lanczos -vf "scale=320x180" -pix_fmt yuv420p12le bear.y4m -strict -1 /usr/local/hi10p/bin/vpxenc --bit-depth=12 --codec=vp9 --profile=2 -o bear-320x180-hi12p-vp9.webm bear.y4m Both ffmpeg and vpxenc were compiled from source to make sure they have highbit support enabled.
Message was sent while issue was closed.
fbarchard@google.com changed reviewers: + fbarchard@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/2370453003/diff/180001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2370453003/diff/180001/cc/resources/video_res... cc/resources/video_resource_updater.cc:295: void VideoResourceUpdater::MakeHalfFloats(const uint16_t* src, libyuv:HalfFloatPlane() implements this function in AVX2, if you'd like to call that instead. The 'bits_per_channel' parameter is replaced with a float multiplier, which is 1.0f / ((1 << bits_per_channel) - 1) for your use case. https://codereview.chromium.org/2370453003/diff/180001/cc/resources/video_res... cc/resources/video_resource_updater.cc:310: dst[i] = (*(uint32_t*)&value) >> 13; When used on 12 bit, the half float will store only 10 bits of mantissa and this shift by 13 will loose bits, rounding with 'truncation' Is truncation acceptable? VCVTPS2PH supports rounding or truncation. http://www.felixcloutier.com/x86/VCVTPS2PH.html If you want to mimic rounding, you could add 0.5 effectively, after the multiply but before the shift. |
