fix build when ffmpeg, libvpx and libwebm are disabled

fix build when ffmpeg, libvpx and libwebm are disabled

Tested with all combinaisons of
GYP_DEFINES="media_use_ffmpeg=i media_use_libvpx=j media_use_libwebm=k"
with i,j,k in {0, 1}^3

Same with GN:
media_use_ffmpeg = i
media_use_libvpx = j
media_use_libwebm = k
with i,j,k in {false, true}^3

If disabled it shows a black rectangle on video tag as expected.
If only ffmpeg is disabled, it succeeds to play webm video encoded with vpx.

BUG=
R=reillyg@chromium.org, jochen@chromium.org, xhwang@chromium.org, raymes@chromium.org
TEST=GYP_DEFINES="media_use_ffmpeg=0 media_use_libvpx=0 media_use_libwebm=0"

Committed: https://crrev.com/f6778e65400fd7b4b9592523c1de811dcde1355a
Cr-Commit-Position: refs/heads/master@{#359855}

[xhwang, 2015-11-02 18:11:51]

Thanks for fixing these. I just have a few comments.

https://codereview.chromium.org/1415793003/diff/1/build/gn_migration.gypi
File build/gn_migration.gypi (right):

https://codereview.chromium.org/1415793003/diff/1/build/gn_migration.gypi#new...
build/gn_migration.gypi:379: 'dependencies': [
nit: Use 2-space indentation.

https://codereview.chromium.org/1415793003/diff/1/chrome/utility/extensions/e...
File chrome/utility/extensions/extensions_handler.cc (right):

https://codereview.chromium.org/1415793003/diff/1/chrome/utility/extensions/e...
chrome/utility/extensions/extensions_handler.cc:24: #include
"media/base/media_file_checker.h"
Move this to ifdef MEDIA_DISABLE_FFMPEG as well.

https://codereview.chromium.org/1415793003/diff/1/chrome/utility/extensions/e...
chrome/utility/extensions/extensions_handler.cc:130: Send(new
ChromeUtilityHostMsg_CheckMediaFile_Finished(false));
I suppose we'll never reach here (since nobody complaints so far), maybe
NOTREACHED()? +Reilly Grant

https://codereview.chromium.org/1415793003/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_decoder.cc (right):

https://codereview.chromium.org/1415793003/diff/1/content/renderer/media/audi...
content/renderer/media/audio_decoder.cc:95: #endif
#endif  // !defined(MEDIA_DISABLE_FFMPEG)

https://codereview.chromium.org/1415793003/diff/1/content/renderer/media/medi...
File content/renderer/media/media_recorder_handler.cc (right):

https://codereview.chromium.org/1415793003/diff/1/content/renderer/media/medi...
content/renderer/media/media_recorder_handler.cc:81: slice_origin_timestamp_ =
TimeTicks::Now();
Move these two lines to below l.83 as well.

https://codereview.chromium.org/1415793003/diff/1/content/renderer/media/medi...
content/renderer/media/media_recorder_handler.cc:83: #if
!defined(MEDIA_DISABLE_LIBWEBM)
+mcasas: shouldn't we exclude media_recorder_hander.cc entirely if
!defined(MEDIA_DISABLE_LIBWEBM)?

https://codereview.chromium.org/1415793003/diff/1/content/renderer/media/medi...
content/renderer/media/media_recorder_handler.cc:84: webm_muxer_.reset(
Do you need to fix the header file as well?

[xhwang, 2015-11-02 18:12:46]

Please also help fix the gn build. You can start with media/BUILD.gn.

[Julien Isorce Samsung, 2015-11-03 16:02:28]

Thx for the review, I'll address remark (see my replies) and I'll also check the
GN build.

https://codereview.chromium.org/1415793003/diff/1/build/gn_migration.gypi
File build/gn_migration.gypi (right):

https://codereview.chromium.org/1415793003/diff/1/build/gn_migration.gypi#new...
build/gn_migration.gypi:379: 'dependencies': [
On 2015/11/02 18:11:51, xhwang wrote:
> nit: Use 2-space indentation.

Acknowledged.

https://codereview.chromium.org/1415793003/diff/1/chrome/utility/extensions/e...
File chrome/utility/extensions/extensions_handler.cc (right):

https://codereview.chromium.org/1415793003/diff/1/chrome/utility/extensions/e...
chrome/utility/extensions/extensions_handler.cc:24: #include
"media/base/media_file_checker.h"
On 2015/11/02 18:11:51, xhwang wrote:
> Move this to ifdef MEDIA_DISABLE_FFMPEG as well.

Acknowledged.

https://codereview.chromium.org/1415793003/diff/1/chrome/utility/extensions/e...
chrome/utility/extensions/extensions_handler.cc:130: Send(new
ChromeUtilityHostMsg_CheckMediaFile_Finished(false));
On 2015/11/02 18:11:51, xhwang wrote:
> I suppose we'll never reach here (since nobody complaints so far), maybe
> NOTREACHED()? +Reilly Grant
You are right, I'll add NOTREACHED() so that it won't be silent if actually
reached.

https://codereview.chromium.org/1415793003/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_decoder.cc (right):

https://codereview.chromium.org/1415793003/diff/1/content/renderer/media/audi...
content/renderer/media/audio_decoder.cc:95: #endif
On 2015/11/02 18:11:51, xhwang wrote:
> #endif  // !defined(MEDIA_DISABLE_FFMPEG)

Acknowledged.

https://codereview.chromium.org/1415793003/diff/1/content/renderer/media/medi...
File content/renderer/media/media_recorder_handler.cc (right):

https://codereview.chromium.org/1415793003/diff/1/content/renderer/media/medi...
content/renderer/media/media_recorder_handler.cc:81: slice_origin_timestamp_ =
TimeTicks::Now();
On 2015/11/02 18:11:51, xhwang wrote:
> Move these two lines to below l.83 as well.
ok, I see it will still work since ::start and ::WriteData are called in the
same thread. (so the 2 calls are serialized and slice_origin_timestamp_ will be
up to date at the time ::WriteData is called)

https://codereview.chromium.org/1415793003/diff/1/content/renderer/media/medi...
content/renderer/media/media_recorder_handler.cc:83: #if
!defined(MEDIA_DISABLE_LIBWEBM)
On 2015/11/02 18:11:51, xhwang wrote:
> +mcasas: shouldn't we exclude media_recorder_hander.cc entirely if
> !defined(MEDIA_DISABLE_LIBWEBM)?
I'll let mcasas decide but I think not disabling media recorder entirely let
someone provides an alternative impl if not using libwebm.

https://codereview.chromium.org/1415793003/diff/1/content/renderer/media/medi...
content/renderer/media/media_recorder_handler.cc:84: webm_muxer_.reset(
On 2015/11/02 18:11:51, xhwang wrote:
> Do you need to fix the header file as well?
Yes I will add the guard. Thx

[Julien Isorce Samsung, 2015-11-05 17:21:29]

Just in case you have not noticed I submitted "patch set 2" that addressed your
remarks (sorry I am quite new to this codereview tool so I am not sure what
triggers notifications or not). I suspect you just had not enough time yet.

Would it be possible to run some try bots ? Also would it be possible to setup
permanently a try bot with GYP_DEFINES="media_use_ffmpeg=0 media_use_libvpx=0
media_use_libwebm=0" (or "media_use_ffmpeg = false media_use_libvpx = false
media_use_libwebm = false" if GN build) ?

Thx again for the reviews.

[xhwang, 2015-11-05 17:27:17]

lgtm

Please also wait for other reviewers' review. Thanks for contributing to
Chromium!

https://codereview.chromium.org/1415793003/diff/1/chrome/utility/extensions/e...
File chrome/utility/extensions/extensions_handler.cc (right):

https://codereview.chromium.org/1415793003/diff/1/chrome/utility/extensions/e...
chrome/utility/extensions/extensions_handler.cc:24: #include
"media/base/media_file_checker.h"
On 2015/11/03 16:02:27, j.isorce wrote:
> On 2015/11/02 18:11:51, xhwang wrote:
> > Move this to ifdef MEDIA_DISABLE_FFMPEG as well.
> 
> Acknowledged.

tip: Use "Done" when it's done :)

https://codereview.chromium.org/1415793003/diff/20001/build/gn_migration.gypi
File build/gn_migration.gypi (right):

https://codereview.chromium.org/1415793003/diff/20001/build/gn_migration.gypi...
build/gn_migration.gypi:182: '../base/base.gyp:base_junit_tests',
tip: next time try to keep rebasing in a different PatchSet, which will make
reviewing a lot easier.

[xhwang, 2015-11-05 17:29:24]

On 2015/11/05 17:21:29, j.isorce wrote:
> Just in case you have not noticed I submitted "patch set 2" that addressed
your
> remarks (sorry I am quite new to this codereview tool so I am not sure what
> triggers notifications or not). I suspect you just had not enough time yet.

I am sorry I missed the message.

> Would it be possible to run some try bots ? Also would it be possible to setup
> permanently a try bot with GYP_DEFINES="media_use_ffmpeg=0 media_use_libvpx=0
> media_use_libwebm=0" (or "media_use_ffmpeg = false media_use_libvpx = false
> media_use_libwebm = false" if GN build) ?
> 
> Thx again for the reviews.

I don't think we have the capacity to excise all gyp define combinations in try
bots...

[Reilly Grant (use Gerrit), 2015-11-05 18:33:06]

https://codereview.chromium.org/1415793003/diff/20001/chrome/utility/extensio...
File chrome/utility/extensions/extensions_handler.cc (right):

https://codereview.chromium.org/1415793003/diff/20001/chrome/utility/extensio...
chrome/utility/extensions/extensions_handler.cc:133: NOTREACHED();
It seems that we should reply with failure here rather than crash the browser.

[Julien Isorce gmail, 2015-11-06 10:17:47]

https://codereview.chromium.org/1415793003/diff/20001/chrome/utility/extensio...
File chrome/utility/extensions/extensions_handler.cc (right):

https://codereview.chromium.org/1415793003/diff/20001/chrome/utility/extensio...
chrome/utility/extensions/extensions_handler.cc:133: NOTREACHED();
On 2015/11/05 18:33:06, Reilly Grant wrote:
> It seems that we should reply with failure here rather than crash the browser.

Done in the new "Patch set 3"

[Julien Isorce Samsung, 2015-11-06 10:20:38]

On 2015/11/06 10:17:47, j.isorce wrote:
>
https://codereview.chromium.org/1415793003/diff/20001/chrome/utility/extensio...
> chrome/utility/extensions/extensions_handler.cc:133: NOTREACHED();
> On 2015/11/05 18:33:06, Reilly Grant wrote:
> > It seems that we should reply with failure here rather than crash the
browser.
> 
> Done in the new "Patch set 3"

I also updated commit message to mention it has been tested for GN too. Though I
do not see the description updated in this page.

[Reilly Grant (use Gerrit), 2015-11-06 19:25:34]

lgtm

[jochen (gone - plz use gerrit), 2015-11-07 16:30:57]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-07 16:31:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415793003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415793003/40001

[commit-bot: I haz the power, 2015-11-07 16:32:45]

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

[commit-bot: I haz the power, 2015-11-07 16:32:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jochen (gone - plz use gerrit), 2015-11-07 16:38:13]

could you rebase the patch please?

[Julien Isorce Samsung, 2015-11-09 11:48:02]

Description was changed from

==========
fix build when ffmpeg, libvpx and libwebm are disabled

Tested with all combinaisons of
GYP_DEFINES="media_use_ffmpeg=i media_use_libvpx=j media_use_libwebm=k"

If disabled it shows a black rectangle on video tag as expected.
If only ffmpeg is disabled, it succeeds to play webm video encoded with vpx.

BUG=

R=reillyg@chromium.org, jochen@chromium.org, xhwang@chromium.org,
raymes@chromium.org

TEST=GYP_DEFINES="media_use_ffmpeg=0 media_use_libvpx=0 media_use_libwebm=0"
==========

to

==========
Tested with all combinaisons of
GYP_DEFINES="media_use_ffmpeg=i media_use_libvpx=j media_use_libwebm=k"
with i,j,k in {0, 1}^3

Same with GN:
media_use_ffmpeg = i
media_use_libvpx = j
media_use_libwebm = k
with i,j,k in {false, true}^3

If disabled it shows a black rectangle on video tag as expected.
If only ffmpeg is disabled, it succeeds to play webm video encoded with vpx.

BUG=

Review URL: https://codereview.chromium.org/1415793003

R=reillyg@chromium.org, jochen@chromium.org, xhwang@chromium.org,
raymes@chromium.org

TEST=GYP_DEFINES="media_use_ffmpeg=0 media_use_libvpx=0 media_use_libwebm=0"
==========

[Julien Isorce Samsung, 2015-11-09 11:55:20]

On 2015/11/07 16:38:13, jochen (slow - traveling) wrote:
> could you rebase the patch please?

Done, I rebased. Also note that I decided to keep "slice_origin_timestamp_ =
TimeTicks::Now();" before the #if block as apposed to what was suggested in
first review. I initially addressed that remark but after rebasing I see that
"media_stream_.videoTracks" is also required.  In the end I found not moving
"slice_origin_timestamp_ =
TimeTicks::Now();" is easier to read. Even if set it will fail "return false"
anyway, like in "if (video_tracks.isEmpty()) {" just below.
Let me know if it is ok otherwise I can move it again. Thx

[xhwang, 2015-11-09 19:19:57]

On 2015/11/09 11:55:20, j.isorce wrote:
> On 2015/11/07 16:38:13, jochen (slow - traveling) wrote:
> > could you rebase the patch please?
> 
> Done, I rebased. Also note that I decided to keep "slice_origin_timestamp_ =
> TimeTicks::Now();" before the #if block as apposed to what was suggested in
> first review. I initially addressed that remark but after rebasing I see that
> "media_stream_.videoTracks" is also required.  In the end I found not moving
> "slice_origin_timestamp_ =
> TimeTicks::Now();" is easier to read. Even if set it will fail "return false"
> anyway, like in "if (video_tracks.isEmpty()) {" just below.
> Let me know if it is ok otherwise I can move it again. Thx

This change sgtm.

[jochen (gone - plz use gerrit), 2015-11-10 20:04:13]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-10 20:05:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415793003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415793003/60001

[commit-bot: I haz the power, 2015-11-10 20:16:08]

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

[commit-bot: I haz the power, 2015-11-10 20:16:09]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jochen (gone - plz use gerrit), 2015-11-11 07:43:14]

Description was changed from

==========
Tested with all combinaisons of
GYP_DEFINES="media_use_ffmpeg=i media_use_libvpx=j media_use_libwebm=k"
with i,j,k in {0, 1}^3

Same with GN:
media_use_ffmpeg = i
media_use_libvpx = j
media_use_libwebm = k
with i,j,k in {false, true}^3

If disabled it shows a black rectangle on video tag as expected.
If only ffmpeg is disabled, it succeeds to play webm video encoded with vpx.

BUG=

Review URL: https://codereview.chromium.org/1415793003

R=reillyg@chromium.org, jochen@chromium.org, xhwang@chromium.org,
raymes@chromium.org

TEST=GYP_DEFINES="media_use_ffmpeg=0 media_use_libvpx=0 media_use_libwebm=0"
==========

to

==========
fix build when ffmpeg, libvpx and libwebm are disabled

Tested with all combinaisons of
GYP_DEFINES="media_use_ffmpeg=i media_use_libvpx=j media_use_libwebm=k"
with i,j,k in {0, 1}^3

Same with GN:
media_use_ffmpeg = i
media_use_libvpx = j
media_use_libwebm = k
with i,j,k in {false, true}^3

If disabled it shows a black rectangle on video tag as expected.
If only ffmpeg is disabled, it succeeds to play webm video encoded with vpx.

BUG=
R=reillyg@chromium.org, jochen@chromium.org, xhwang@chromium.org,
raymes@chromium.org
TEST=GYP_DEFINES="media_use_ffmpeg=0 media_use_libvpx=0 media_use_libwebm=0"
==========

[jochen (gone - plz use gerrit), 2015-11-11 07:43:20]

lgtm

[Julien Isorce Samsung, 2015-11-13 15:42:08]

On 2015/11/11 07:43:20, jochen (slow - traveling) wrote:
> lgtm

Thx for the reviews. Do you need any action from me ? Not sure what to do next
to land the patch :)

[jochen (gone - plz use gerrit), 2015-11-16 11:53:50]

On 2015/11/13 at 15:42:08, j.isorce wrote:
> On 2015/11/11 07:43:20, jochen (slow - traveling) wrote:
> > lgtm
> 
> Thx for the reviews. Do you need any action from me ? Not sure what to do next
to land the patch :)

they presubmit step is failing:
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...

the output says that you're not in the AUTHORS file. you'll need to add yourself
there. once that's done (ok to add this in this CL), hit the "Commit" button on
this CL

[Julien Isorce gmail, 2015-11-16 13:59:24]

ticked "commit" but seems to fail without any particular message.

[Julien Isorce Samsung, 2015-11-16 14:00:43]

The CQ bit was checked by j.isorce@samsung.com

[Julien Isorce Samsung, 2015-11-16 14:00:43]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
reillyg@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1415793003/#ps80001
(title: " ")

[Julien Isorce Samsung, 2015-11-16 14:01:18]

Ticked while signed it with correct account

[commit-bot: I haz the power, 2015-11-16 14:01:45]

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

[commit-bot: I haz the power, 2015-11-16 17:14:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-16 17:15:13]

Patchset 5 (id:??) landed as
https://crrev.com/f6778e65400fd7b4b9592523c1de811dcde1355a
Cr-Commit-Position: refs/heads/master@{#359855}