ffmpeg chromium glue

Glue ffmpeg to Chrome
1) works for Linux, OSX, Windows
2) mostly renaming #defines
3) avcodec_thread_init is deprecated and has been removed, see
   http://ffmpeg.org/pipermail/ffmpeg-cvslog/2011-February/034326.html
4) will require a rebaseline of 
   platform/chromium-mac/media/video-currentTime-set-expected.txt
   platform/chromium-win/media/video-currentTime-set-expected.txt
   with a change of
   -video.currentTime.toFixed(2) == '5.82'
   +video.currentTime.toFixed(2) == '5.81'


BUG=87831
TESTS=ran Theora Testsuite, media tests, WebKit media tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91259

[ilja, 2011-06-29 01:19:29]

Notice that I previously had modified
media/tools/omx_test/file_reader_util.cc
media/tools/omx_test/omx_test.cc
but scherkus seems to have removed them.

Otherwise we have to allocate memory now for format_context_.streams in
ffmpeg_demuxer_unittest.cc but it seems av_malloc is not happy (I don't get a
good stack in the crash). New works fine though but seems wrong to me. Any idea?

[scherkus (not reviewing), 2011-06-29 17:00:35]

looking pretty slick!

few nits

also I'm wary of adding some of the video/x-msvideo / avi stuff in this patch

might be better suited in a follow up patch to make sure we get that stuff right

http://codereview.chromium.org/6993042/diff/48008/chrome/browser/extensions/f...
File chrome/browser/extensions/file_manager_util.cc (right):

http://codereview.chromium.org/6993042/diff/48008/chrome/browser/extensions/f...
chrome/browser/extensions/file_manager_util.cc:42: ".mp3", ".m4a", ".ogg",
".oga", ".wav", ".xvid",
I thought we weren't doing divx/xvid extensions?

let's get zelidrag onto this CL

http://codereview.chromium.org/6993042/diff/48008/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/6993042/diff/48008/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:156: 'video':
/\.(avi|divx|mov|mp4|m4v|mpe?g4?|ogm|ogv|ogx|webm|xvid)$/i
ditto

http://codereview.chromium.org/6993042/diff/48008/chrome/browser/ui/webui/fil...
File chrome/browser/ui/webui/fileicon_source_cros.cc (right):

http://codereview.chromium.org/6993042/diff/48008/chrome/browser/ui/webui/fil...
chrome/browser/ui/webui/fileicon_source_cros.cc:149: std::make_pair(".xvid",
kVideoIdrs),
ditto

http://codereview.chromium.org/6993042/diff/48008/chrome_frame/protocol_sink_...
File chrome_frame/protocol_sink_wrap.cc (right):

http://codereview.chromium.org/6993042/diff/48008/chrome_frame/protocol_sink_...
chrome_frame/protocol_sink_wrap.cc:231: "video/x-msvideo",
since we're only adding these additional codecs for chrome os, we shouldn't need
to update chrome frame (win only)

...but we'll get someone from chrome frame team to weigh in here

at a minimum it should be wrapped in OS_CHROME #ifdef

http://codereview.chromium.org/6993042/diff/48008/media/base/mock_ffmpeg.cc
File media/base/mock_ffmpeg.cc (right):

http://codereview.chromium.org/6993042/diff/48008/media/base/mock_ffmpeg.cc#n...
media/base/mock_ffmpeg.cc:115: //int avcodec_thread_init(AVCodecContext* avctx,
int threads) {
was the function signature changed?

can we get rid of this?

http://codereview.chromium.org/6993042/diff/48008/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/6993042/diff/48008/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:466: // Remove ourself from protocol list.
could you add back this blank line?

http://codereview.chromium.org/6993042/diff/48008/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/6993042/diff/48008/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_unittest.cc:107: //    av_malloc(sizeof(AVStream *)
* AV_STREAM_MAX));
this is because media_unittests doesn't load the real FFmpeg (it uses MockFFmpeg
-- which TBH is mocking gone overboard)

so using new is ok here, but make sure to call delete as well :)

http://codereview.chromium.org/6993042/diff/48008/media/test/ffmpeg_tests/ffm...
File media/test/ffmpeg_tests/ffmpeg_tests.cc (right):

http://codereview.chromium.org/6993042/diff/48008/media/test/ffmpeg_tests/ffm...
media/test/ffmpeg_tests/ffmpeg_tests.cc:237: // avcodec_thread_init deprecated
you can remove this comment

http://codereview.chromium.org/6993042/diff/48008/media/tools/media_bench/med...
File media/tools/media_bench/media_bench.cc (right):

http://codereview.chromium.org/6993042/diff/48008/media/tools/media_bench/med...
media/tools/media_bench/media_bench.cc:344: // avcodec_thread_init deprecated
ditto

http://codereview.chromium.org/6993042/diff/48008/media/video/ffmpeg_video_de...
File media/video/ffmpeg_video_decode_engine_unittest.cc (right):

http://codereview.chromium.org/6993042/diff/48008/media/video/ffmpeg_video_de...
media/video/ffmpeg_video_decode_engine_unittest.cc:90: //       
.WillOnce(Return(0));
you can remove

http://codereview.chromium.org/6993042/diff/48008/media/video/ffmpeg_video_de...
media/video/ffmpeg_video_decode_engine_unittest.cc:194: (InitThread can't fail
anymore as is deprecated)
just nuke the test :)

http://codereview.chromium.org/6993042/diff/48008/media/video/ffmpeg_video_de...
media/video/ffmpeg_video_decode_engine_unittest.cc:229: //     
.WillOnce(Return(0));
ditto

http://codereview.chromium.org/6993042/diff/48008/net/base/platform_mime_util...
File net/base/platform_mime_util_linux.cc (right):

http://codereview.chromium.org/6993042/diff/48008/net/base/platform_mime_util...
net/base/platform_mime_util_linux.cc:55: {"video/x-msvideo", "avi"},
woah I've never seen this mime typ mapping struct before... what's it used for?

http://codereview.chromium.org/6993042/diff/48008/webkit/tools/layout_tests/t...
File webkit/tools/layout_tests/test_expectations.txt (right):

http://codereview.chromium.org/6993042/diff/48008/webkit/tools/layout_tests/t...
webkit/tools/layout_tests/test_expectations.txt:20: BUGCR87831 MAC :
media/video-currentTime-set.html = TEXT
you can ignore the OS portion and rewrite this as:

BUGCR87831 : media/video-currentTime-set.html

[rbultje, 2011-06-29 17:43:16]

Good stuff - other than scherkus' comments, nothing more from me. Great work!

[fbarchard, 2011-06-29 18:28:27]

Looking good.  I went over some of the changes with Ronald offline and he
think's you've done it right.

http://codereview.chromium.org/6993042/diff/48008/media/base/mock_ffmpeg.cc
File media/base/mock_ffmpeg.cc (right):

http://codereview.chromium.org/6993042/diff/48008/media/base/mock_ffmpeg.cc#n...
media/base/mock_ffmpeg.cc:115: //int avcodec_thread_init(AVCodecContext* avctx,
int threads) {
code review for ffmpeg said
"they should be renamed to ff_ and wrapers removed once the major ver is bumped"

http://codereview.chromium.org/6993042/diff/48008/media/video/ffmpeg_video_de...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/6993042/diff/48008/media/video/ffmpeg_video_de...
media/video/ffmpeg_video_decode_engine.cc:130: codec_context_->thread_count =
decode_threads;
can we remove avcodec_thread_init()?

[ilja, 2011-06-29 21:40:05]

Zel: could you please respond to Andrews questions if you want divx and xvid in
CrOS filebrowser only? There seem to be conflicting opinions.

I am keeping this in for now. Otherwise I am removing all mime "video/x-msvideo"
references for now. The rest is cleanup as suggested.

During syncing to ToT I had to resolve three new files:
      media/ffmpeg/ffmpeg_common.cc
      media/filters/chunk_demuxer.cc
      media/filters/chunk_demuxer_unittest.cc

Finally I can't land this without a simultaneous webkit checkin to fix the
patched-ffmpeg(-mt) path. How do we do that?

http://codereview.chromium.org/6993042/diff/48008/chrome/browser/extensions/f...
File chrome/browser/extensions/file_manager_util.cc (right):

http://codereview.chromium.org/6993042/diff/48008/chrome/browser/extensions/f...
chrome/browser/extensions/file_manager_util.cc:42: ".mp3", ".m4a", ".ogg",
".oga", ".wav", ".xvid",
On 2011/06/29 17:00:36, scherkus wrote:
> I thought we weren't doing divx/xvid extensions?
> 
> let's get zelidrag onto this CL

Inviting him. These extensions are only for the file browser in CrOS. And the
last I heard Zel wanted all three.

http://codereview.chromium.org/6993042/diff/48008/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/6993042/diff/48008/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:156: 'video':
/\.(avi|divx|mov|mp4|m4v|mpe?g4?|ogm|ogv|ogx|webm|xvid)$/i
On 2011/06/29 17:00:36, scherkus wrote:
> ditto

Zel?

http://codereview.chromium.org/6993042/diff/48008/chrome/browser/ui/webui/fil...
File chrome/browser/ui/webui/fileicon_source_cros.cc (right):

http://codereview.chromium.org/6993042/diff/48008/chrome/browser/ui/webui/fil...
chrome/browser/ui/webui/fileicon_source_cros.cc:149: std::make_pair(".xvid",
kVideoIdrs),
On 2011/06/29 17:00:36, scherkus wrote:
> ditto

Zel again?

http://codereview.chromium.org/6993042/diff/48008/chrome_frame/protocol_sink_...
File chrome_frame/protocol_sink_wrap.cc (right):

http://codereview.chromium.org/6993042/diff/48008/chrome_frame/protocol_sink_...
chrome_frame/protocol_sink_wrap.cc:231: "video/x-msvideo",
On 2011/06/29 17:00:36, scherkus wrote:
> since we're only adding these additional codecs for chrome os, we shouldn't
need
> to update chrome frame (win only)
> 
> ...but we'll get someone from chrome frame team to weigh in here
> 
> at a minimum it should be wrapped in OS_CHROME #ifdef

I thought about this and it seems I don't need mime types for CrOS. I am
removing all references and will add them in a later change if I am mistaken.

http://codereview.chromium.org/6993042/diff/48008/media/base/mock_ffmpeg.cc
File media/base/mock_ffmpeg.cc (right):

http://codereview.chromium.org/6993042/diff/48008/media/base/mock_ffmpeg.cc#n...
media/base/mock_ffmpeg.cc:115: //int avcodec_thread_init(AVCodecContext* avctx,
int threads) {
On 2011/06/29 17:00:36, scherkus wrote:
> was the function signature changed?
> 
> can we get rid of this?

Done.

http://codereview.chromium.org/6993042/diff/48008/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/6993042/diff/48008/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:466: // Remove ourself from protocol list.
On 2011/06/29 17:00:36, scherkus wrote:
> could you add back this blank line?

Done.

http://codereview.chromium.org/6993042/diff/48008/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/6993042/diff/48008/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer_unittest.cc:107: //    av_malloc(sizeof(AVStream *)
* AV_STREAM_MAX));
On 2011/06/29 17:00:36, scherkus wrote:
> this is because media_unittests doesn't load the real FFmpeg (it uses
MockFFmpeg
> -- which TBH is mocking gone overboard)
> 
> so using new is ok here, but make sure to call delete as well :)

Done.

http://codereview.chromium.org/6993042/diff/48008/media/test/ffmpeg_tests/ffm...
File media/test/ffmpeg_tests/ffmpeg_tests.cc (right):

http://codereview.chromium.org/6993042/diff/48008/media/test/ffmpeg_tests/ffm...
media/test/ffmpeg_tests/ffmpeg_tests.cc:237: // avcodec_thread_init deprecated
On 2011/06/29 17:00:36, scherkus wrote:
> you can remove this comment

Done.

http://codereview.chromium.org/6993042/diff/48008/media/tools/media_bench/med...
File media/tools/media_bench/media_bench.cc (right):

http://codereview.chromium.org/6993042/diff/48008/media/tools/media_bench/med...
media/tools/media_bench/media_bench.cc:344: // avcodec_thread_init deprecated
On 2011/06/29 17:00:36, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/6993042/diff/48008/media/video/ffmpeg_video_de...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/6993042/diff/48008/media/video/ffmpeg_video_de...
media/video/ffmpeg_video_decode_engine.cc:130: codec_context_->thread_count =
decode_threads;
On 2011/06/29 18:28:27, fbarchard wrote:
> can we remove avcodec_thread_init()?

Well, it is deprecated, so I am removing it everywhere. But I guess I should
still remove it separately in the signature file. Anywhere else?

http://codereview.chromium.org/6993042/diff/48008/media/video/ffmpeg_video_de...
File media/video/ffmpeg_video_decode_engine_unittest.cc (right):

http://codereview.chromium.org/6993042/diff/48008/media/video/ffmpeg_video_de...
media/video/ffmpeg_video_decode_engine_unittest.cc:90: //       
.WillOnce(Return(0));
On 2011/06/29 17:00:36, scherkus wrote:
> you can remove

Done.

http://codereview.chromium.org/6993042/diff/48008/media/video/ffmpeg_video_de...
media/video/ffmpeg_video_decode_engine_unittest.cc:194: (InitThread can't fail
anymore as is deprecated)
On 2011/06/29 17:00:36, scherkus wrote:
> just nuke the test :)

Done.

http://codereview.chromium.org/6993042/diff/48008/media/video/ffmpeg_video_de...
media/video/ffmpeg_video_decode_engine_unittest.cc:229: //     
.WillOnce(Return(0));
On 2011/06/29 17:00:36, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/6993042/diff/48008/net/base/platform_mime_util...
File net/base/platform_mime_util_linux.cc (right):

http://codereview.chromium.org/6993042/diff/48008/net/base/platform_mime_util...
net/base/platform_mime_util_linux.cc:55: {"video/x-msvideo", "avi"},
On 2011/06/29 17:00:36, scherkus wrote:
> woah I've never seen this mime typ mapping struct before... what's it used
for?

I think it is for displaying files in the download tab. They have an icon next
to them and the code guesses the mime type independently of the OS. Now we
actually handle this differently now, so removing.

http://codereview.chromium.org/6993042/diff/48008/webkit/tools/layout_tests/t...
File webkit/tools/layout_tests/test_expectations.txt (right):

http://codereview.chromium.org/6993042/diff/48008/webkit/tools/layout_tests/t...
webkit/tools/layout_tests/test_expectations.txt:20: BUGCR87831 MAC :
media/video-currentTime-set.html = TEXT
On 2011/06/29 17:00:36, scherkus wrote:
> you can ignore the OS portion and rewrite this as:
> 
> BUGCR87831 : media/video-currentTime-set.html

Done.

[zel, 2011-06-29 22:09:11]

http://codereview.chromium.org/6993042/diff/48008/chrome/browser/extensions/f...
File chrome/browser/extensions/file_manager_util.cc (right):

http://codereview.chromium.org/6993042/diff/48008/chrome/browser/extensions/f...
chrome/browser/extensions/file_manager_util.cc:42: ".mp3", ".m4a", ".ogg",
".oga", ".wav", ".xvid",
On 2011/06/29 21:40:05, ihf wrote:
> On 2011/06/29 17:00:36, scherkus wrote:
> > I thought we weren't doing divx/xvid extensions?
> > 
> > let's get zelidrag onto this CL
> 
> Inviting him. These extensions are only for the file browser in CrOS. And the
> last I heard Zel wanted all three.

Let's remove support for .divx and .xvid file format container and stick with
.avi only.

[ilja, 2011-06-29 22:17:45]

Removed divx/xdiv. I am not running new bots though as the results for patch set
8 should still be good.

[fbarchard, 2011-06-29 22:21:05]

http://codereview.chromium.org/6993042/diff/50009/media/tools/media_bench/med...
File media/tools/media_bench/media_bench.cc (right):

http://codereview.chromium.org/6993042/diff/50009/media/tools/media_bench/med...
media/tools/media_bench/media_bench.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
// Copyright (c) 2011
or do these get updated automagically now?

http://codereview.chromium.org/6993042/diff/50009/media/video/ffmpeg_video_de...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/6993042/diff/50009/media/video/ffmpeg_video_de...
media/video/ffmpeg_video_decode_engine.cc:89: 1 : kDecodeThreads;
Can you test if this is still necessary?

[fbarchard, 2011-06-29 22:22:47]

http://codereview.chromium.org/6993042/diff/50009/DEPS
File DEPS (right):

http://codereview.chromium.org/6993042/diff/50009/DEPS#newcode18
DEPS:18: "ffmpeg_revision": "90064",
Are the chromium windows binaries updated in 90064?

[ilja, 2011-06-30 01:28:58]

http://codereview.chromium.org/6993042/diff/50009/DEPS
File DEPS (right):

http://codereview.chromium.org/6993042/diff/50009/DEPS#newcode18
DEPS:18: "ffmpeg_revision": "90064",
On 2011/06/29 22:22:47, fbarchard wrote:
> Are the chromium windows binaries updated in 90064?

Actually 90062, but should be ok.

http://codereview.chromium.org/6993042/diff/50009/media/tools/media_bench/med...
File media/tools/media_bench/media_bench.cc (right):

http://codereview.chromium.org/6993042/diff/50009/media/tools/media_bench/med...
media/tools/media_bench/media_bench.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2011/06/29 22:21:06, fbarchard wrote:
> // Copyright (c) 2011
> or do these get updated automagically now?

Done.

http://codereview.chromium.org/6993042/diff/50009/media/video/ffmpeg_video_de...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/6993042/diff/50009/media/video/ffmpeg_video_de...
media/video/ffmpeg_video_decode_engine.cc:89: 1 : kDecodeThreads;
On 2011/06/29 22:21:06, fbarchard wrote:
> Can you test if this is still necessary?

I tried and it seems it is not. But we leave this for next roll.

[scherkus_google.com, 2011-07-01 00:45:32]

LGTM

[fbarchard, 2011-07-01 00:48:46]

LGTM

[jamesr, 2011-07-01 20:20:34]

With this patch applied most (or possibly all) of the WebKit GPU tests involving
video are timing out on windows.

[jamesr, 2011-07-01 20:25:59]

On 2011/07/01 20:20:34, jamesr wrote:
> With this patch applied most (or possibly all) of the WebKit GPU tests
involving
> video are timing out on windows.

Here are the bots that show failing tests, it's possible that they might just
need a clobber:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Vista
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win7

[jamesr, 2011-07-01 20:27:51]

Dashboard link for the tests timing out:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=com...

[ihf, 2011-07-01 21:06:32]

The two bots you are listing show as one of the first failures videoBear.
This (and variations) were failures we saw yesterday evening on Windows
machines and they went away after clobber. That said I don't know how to
clobber them myself or if mihaip already did so via bug
http://code.google.com/p/chromium/issues/detail?id=88197

Ilja.

On Fri, Jul 1, 2011 at 1:25 PM, <jamesr@chromium.org> wrote:

> On 2011/07/01 20:20:34, jamesr wrote:
>
>> With this patch applied most (or possibly all) of the WebKit GPU tests
>>
> involving
>
>> video are timing out on windows.
>>
>
> Here are the bots that show failing tests, it's possible that they might
> just
> need a clobber:
>
>
http://build.chromium.org/p/**chromium.webkit/builders/**Webkit%20Vista<http:...
>
http://build.chromium.org/p/**chromium.webkit/builders/**Webkit%20Win7<http:/...
>
>
>
http://codereview.chromium.**org/6993042/<http://codereview.chromium.org/6993...
>