Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(14)

Issue 11468018: Add libvpx wrapper to media to support VP9 video decoding. (Closed)

Created:
8 years ago by Tom Finegan
Modified:
8 years ago
CC:
chromium-reviews, feature-media-reviews_chromium.org, darin-cc_chromium.org
Visibility:
Public.

Description

Add wrapper class to media for support of VP9 video, and add a command line flag to enable the support. This initial version of the wrapper provides support for decoding VP9 video in WebM container files, and is disabled by default. New flag added: --enable-vp9-playback BUG=166094 TEST=VP9 video in WebM containers plays back in <video> elements when --enable-vp9-playback is specified on the command line. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174311

Patch Set 1 #

Patch Set 2 : First pass. Works on Windows, should work elsewhere. #

Total comments: 10

Patch Set 3 : Rebase on 11416367 (opus wrapper), restrict vpx wrapper to vp9, and add canPlayType support #

Patch Set 4 : Address comments. #

Total comments: 18

Patch Set 5 : Address more comments. #

Patch Set 6 : Uncomment VP9 stuff in ffmpeg_common. #

Total comments: 38

Patch Set 7 : Rebase on Opus decoder CL (11416367). #

Patch Set 8 : Remove canPlayType stuff, address comments, and add flag. #

Patch Set 9 : Rebased. Added workaround for lack of pixel format, width, and height from FFmpeg for VP9. #

Patch Set 10 : Use correct codec interface for call to vpx_codec_dec_init(). #

Patch Set 11 : Add work around for lack of VP9 in ffmpeg. #

Total comments: 8

Patch Set 12 : Rebase on opus decoder (11416367 patch 16) and address nits. #

Patch Set 13 : Disable VP9 support on Android and iOS. #

Patch Set 14 : Rebased. #

Total comments: 2

Patch Set 15 : Fixed nit. #

Patch Set 16 : Roll DEPS for libvpx to pick up VP9 support. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+245 lines, -291 lines) Patch
M DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -1 line 0 comments Download
M chrome/app/generated_resources.grd View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +7 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M media/DEPS View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M media/base/media_switches.h View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M media/base/media_switches.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download
M media/base/video_decoder_config.h View 1 2 2 chunks +6 lines, -2 lines 0 comments Download
M media/ffmpeg/ffmpeg_common.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +31 lines, -3 lines 0 comments Download
A + media/filters/vpx_video_decoder.h View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +24 lines, -33 lines 0 comments Download
A + media/filters/vpx_video_decoder.cc View 1 2 3 4 5 6 7 8 9 10 11 12 9 chunks +140 lines, -252 lines 0 comments Download
M media/media.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +16 lines, -0 lines 0 comments Download
M webkit/media/filter_helpers.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +7 lines, -0 lines 0 comments Download

Messages

Total messages: 22 (0 generated)
Tom Finegan
This works on my machine... Still needs some clean up, but reasonable at this point. ...
8 years ago (2012-12-09 23:04:44 UTC) #1
xhwang
drive-by: Didn't review the CL yet; just have some general comments/questions. It seems with this ...
8 years ago (2012-12-10 01:41:11 UTC) #2
fgalligan1
You should probably talk more about how we are trying to add VP9 to Chrome ...
8 years ago (2012-12-10 07:47:12 UTC) #3
Tom Finegan
On 2012/12/10 07:47:12, fgalligan1 wrote: > You should probably talk more about how we are ...
8 years ago (2012-12-11 07:56:40 UTC) #4
Tom Finegan
Still not really ready for review, but initial comments addressed. Rebased on the Opus wrapper ...
8 years ago (2012-12-13 09:57:26 UTC) #5
fgalligan1
https://codereview.chromium.org/11468018/diff/18001/media/filters/vpx_video_decoder.cc File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/11468018/diff/18001/media/filters/vpx_video_decoder.cc#newcode141 media/filters/vpx_video_decoder.cc:141: delete vpx_codec_; You could set vpx_codec_ to NULL here ...
8 years ago (2012-12-13 20:17:30 UTC) #6
Tom Finegan
Addressed comments. https://codereview.chromium.org/11468018/diff/18001/media/filters/vpx_video_decoder.cc File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/11468018/diff/18001/media/filters/vpx_video_decoder.cc#newcode141 media/filters/vpx_video_decoder.cc:141: delete vpx_codec_; On 2012/12/13 20:17:31, fgalligan1 wrote: ...
8 years ago (2012-12-14 01:52:31 UTC) #7
scherkus (not reviewing)
https://codereview.chromium.org/11468018/diff/21013/media/base/video_decoder_config.h File media/base/video_decoder_config.h (right): https://codereview.chromium.org/11468018/diff/21013/media/base/video_decoder_config.h#newcode62 media/base/video_decoder_config.h:62: VP9PROFILE_MIN = 12, do you have to update webkit/plugins/ppapi/ppb_video_decoder_impl.cc ...
8 years ago (2012-12-14 21:32:40 UTC) #8
Tom Finegan
ben: PTAL at flag stuff. Thanks! Media reviewers, PTAL. I think all comments are addressed, ...
8 years ago (2012-12-15 06:13:30 UTC) #9
Tom Finegan
+brettw for OWNERS review of command line flag changes. PTAL Thanks!
8 years ago (2012-12-17 21:17:23 UTC) #10
brettw
owners lgtm rubberstamp
8 years ago (2012-12-17 21:33:32 UTC) #11
Tom Finegan
Made CL description more informative.
8 years ago (2012-12-17 21:33:41 UTC) #12
scherkus (not reviewing)
lgtm w/ nits https://codereview.chromium.org/11468018/diff/36001/media/ffmpeg/ffmpeg_common.cc File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/11468018/diff/36001/media/ffmpeg/ffmpeg_common.cc#newcode324 media/ffmpeg/ffmpeg_common.cc:324: VideoCodecProfile profile = (codec == kCodecVP8) ...
8 years ago (2012-12-18 01:01:55 UTC) #13
Tom Finegan
Thanks for reviews! CQ'ing this after 11416367 lands. https://codereview.chromium.org/11468018/diff/36001/media/ffmpeg/ffmpeg_common.cc File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/11468018/diff/36001/media/ffmpeg/ffmpeg_common.cc#newcode324 media/ffmpeg/ffmpeg_common.cc:324: VideoCodecProfile ...
8 years ago (2012-12-18 01:47:42 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11468018/39003
8 years ago (2012-12-18 07:36:02 UTC) #15
fgalligan1
On 2012/12/18 07:36:02, I haz the power (commit-bot) wrote: > CQ is trying da patch. ...
8 years ago (2012-12-18 15:29:40 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11468018/51001
8 years ago (2012-12-18 17:02:55 UTC) #17
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
8 years ago (2012-12-18 17:13:47 UTC) #18
scherkus (not reviewing)
still lgtm w/ one nit https://codereview.chromium.org/11468018/diff/53008/media/ffmpeg/ffmpeg_common.cc File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/11468018/diff/53008/media/ffmpeg/ffmpeg_common.cc#newcode160 media/ffmpeg/ffmpeg_common.cc:160: } else else isn't ...
8 years ago (2012-12-20 20:19:03 UTC) #19
Tom Finegan
https://codereview.chromium.org/11468018/diff/53008/media/ffmpeg/ffmpeg_common.cc File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/11468018/diff/53008/media/ffmpeg/ffmpeg_common.cc#newcode160 media/ffmpeg/ffmpeg_common.cc:160: } else On 2012/12/20 20:19:03, scherkus wrote: > else ...
8 years ago (2012-12-20 20:37:06 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11468018/60016
8 years ago (2012-12-20 21:54:52 UTC) #21
commit-bot: I haz the power
8 years ago (2012-12-21 01:39:02 UTC) #22
Message was sent while issue was closed.
Change committed as 174311

Powered by Google App Engine
This is Rietveld 408576698