fix for builds with webrtc disabled

fix for builds with webrtc disabled

PepperVideoCaptureHost::OnFrameReady makes use of kUnknownFrameRate
which was previously only included in webrtc builds.  Let's move
these constants to the header file instead, since it can be
included in both webrtc and non-webrtc builds.

Followup to https://codereview.chromium.org/955253002.

BUG=461116, 463829
Committed: https://crrev.com/4ff9bc71c31a9abee29d161acec2872b27b528ad
Cr-Commit-Position: refs/heads/master@{#319169}

[Mostyn Bramley-Moore, 2015-03-04 01:00:13]

mostynb@opera.com changed reviewers:
+ dalecurtis@chromium.org, miu@chromium.org, sievers@chromium.org

[Mostyn Bramley-Moore, 2015-03-04 01:01:53]

@DaleCurtis, sievers, miu: please take a look at this small followup to
https://codereview.chromium.org/955253002

I'm not sure if there's a more sensible place to split this code into
webrtc-only and common blocks.

[miu, 2015-03-04 01:28:56]

lgtm.  Sorry for the churn caused by my change.

Out of curiosity, why is media_stream_video_source.cc only supposed to be
included in WebRTC builds?  Seems like this file contains nothing
WebRTC-specific and is relevant to any producers/consumers around the
getUserMedia() and desktop/tab capture APIs.  What happens if the .cc file is
included in non-WebRTC builds?

[Mostyn Bramley-Moore, 2015-03-04 01:35:41]

On 2015/03/04 01:28:56, miu wrote:
> lgtm.  Sorry for the churn caused by my change.
> 
> Out of curiosity, why is media_stream_video_source.cc only supposed to be
> included in WebRTC builds?  Seems like this file contains nothing
> WebRTC-specific and is relevant to any producers/consumers around the
> getUserMedia() and desktop/tab capture APIs.  What happens if the .cc file is
> included in non-WebRTC builds?

I tried moving media_stream_video_source.cc to the private_renderer_sources list
earlier, but that gives a bunch of other unresolved symbols when webrtc is
disabled:

FAILED: if [ ! -e lib/libcontent.so -o ! -e lib/libcontent.so.TOC ]; then ccache
g++ -shared -Wl,-z,now -Wl,-z,relro -Wl,--fatal-warnings -Wl,-z,defs -pthread
-Wl,-z,noexecstack -fPIC -Wl,--disable-new-dtags -L.
-Wl,-uIsHeapProfilerRunning,-uProfilerStart
-Wl,-u_Z21InitialMallocHook_NewPKvj,-u_Z22InitialMallocHook_MMapPKvS0_jiiix,-u_Z22InitialMallocHook_SbrkPKvi
-Wl,-u_Z21InitialMallocHook_NewPKvm,-u_Z22InitialMallocHook_MMapPKvS0_miiil,-u_Z22InitialMallocHook_SbrkPKvl
-Wl,-u_ZN15HeapLeakChecker12IgnoreObjectEPKv,-u_ZN15HeapLeakChecker14UnIgnoreObjectEPKv
-m64 -Wl,-O1 -Wl,--as-needed -Wl,--gc-sections -o lib/libcontent.so
-Wl,-soname=libcontent.so @lib/libcontent.so.rsp && { readelf -d
lib/libcontent.so | grep SONAME ; nm -gD -f p lib/libcontent.so | cut -f1-2 -d'
'; } > lib/libcontent.so.TOC; else ccache g++ -shared -Wl,-z,now -Wl,-z,relro
-Wl,--fatal-warnings -Wl,-z,defs -pthread -Wl,-z,noexecstack -fPIC
-Wl,--disable-new-dtags -L. -Wl,-uIsHeapProfilerRunning,-uProfilerStart
-Wl,-u_Z21InitialMallocHook_NewPKvj,-u_Z22InitialMallocHook_MMapPKvS0_jiiix,-u_Z22InitialMallocHook_SbrkPKvi
-Wl,-u_Z21InitialMallocHook_NewPKvm,-u_Z22InitialMallocHook_MMapPKvS0_miiil,-u_Z22InitialMallocHook_SbrkPKvl
-Wl,-u_ZN15HeapLeakChecker12IgnoreObjectEPKv,-u_ZN15HeapLeakChecker14UnIgnoreObjectEPKv
-m64 -Wl,-O1 -Wl,--as-needed -Wl,--gc-sections -o lib/libcontent.so
-Wl,-soname=libcontent.so @lib/libcontent.so.rsp && { readelf -d
lib/libcontent.so | grep SONAME ; nm -gD -f p lib/libcontent.so | cut -f1-2 -d'
'; } > lib/libcontent.so.tmp && if ! cmp -s lib/libcontent.so.tmp
lib/libcontent.so.TOC; then mv lib/libcontent.so.tmp lib/libcontent.so.TOC ; fi;
fi
obj/content/renderer/media/content.media_stream_video_source.o: In function
`content::MediaStreamVideoSource::DoStopSource()':
media_stream_video_source.cc:(.text._ZN7content22MediaStreamVideoSource12DoStopSourceEv+0x2a):
undefined reference to `content::VideoTrackAdapter::StopFrameMonitoring()'
obj/content/renderer/media/content.media_stream_video_source.o: In function
`content::MediaStreamVideoSource::SetReadyState(blink::WebMediaStreamSource::ReadyState)':
media_stream_video_source.cc:(.text._ZN7content22MediaStreamVideoSource13SetReadyStateEN5blink20WebMediaStreamSource10ReadyStateE+0x7b):
undefined reference to
`content::MediaStreamVideoTrack::OnReadyStateChanged(blink::WebMediaStreamSource::ReadyState)'
obj/content/renderer/media/content.media_stream_video_source.o: In function
`content::(anonymous
namespace)::UpdateFormatForConstraint(blink::WebMediaConstraint const&, bool,
media::VideoCaptureFormat*)':
media_stream_video_source.cc:(.text._ZN7content12_GLOBAL__N_125UpdateFormatForConstraintERKN5blink18WebMediaConstraintEbPN5media18VideoCaptureFormatE+0xe3):
undefined reference to `content::MediaStreamSource::kSourceId'
obj/content/renderer/media/content.media_stream_video_source.o: In function
`content::MediaStreamVideoSource::MediaStreamVideoSource()':
media_stream_video_source.cc:(.text._ZN7content22MediaStreamVideoSourceC2Ev+0xc):
undefined reference to `content::MediaStreamSource::MediaStreamSource()'
media_stream_video_source.cc:(.text._ZN7content22MediaStreamVideoSourceC2Ev+0xdd):
undefined reference to
`content::VideoTrackAdapter::VideoTrackAdapter(scoped_refptr<base::MessageLoopProxy>
const&)'
obj/content/renderer/media/content.media_stream_video_source.o: In function
`content::MediaStreamVideoSource::~MediaStreamVideoSource()':
media_stream_video_source.cc:(.text._ZN7content22MediaStreamVideoSourceD2Ev+0xdf):
undefined reference to `content::MediaStreamSource::~MediaStreamSource()'
obj/content/renderer/media/content.media_stream_video_source.o: In function
`content::MediaStreamVideoSource::RemoveTrack(content::MediaStreamVideoTrack*)':
media_stream_video_source.cc:(.text._ZN7content22MediaStreamVideoSource11RemoveTrackEPNS_21MediaStreamVideoTrackE+0xc3):
undefined reference to
`content::VideoTrackAdapter::RemoveTrack(content::MediaStreamVideoTrack const*)'
media_stream_video_source.cc:(.text._ZN7content22MediaStreamVideoSource11RemoveTrackEPNS_21MediaStreamVideoTrackE+0x1e4):
undefined reference to `content::MediaStreamSource::StopSource()'
obj/content/renderer/media/content.media_stream_video_source.o: In function
`content::MediaStreamVideoSource::FinalizeAddTrack()':
media_stream_video_source.cc:(.text._ZN7content22MediaStreamVideoSource16FinalizeAddTrackEv+0x394):
undefined reference to
`content::VideoTrackAdapter::AddTrack(content::MediaStreamVideoTrack const*,
base::Callback<void (scoped_refptr<media::VideoFrame> const&, base::TimeTicks
const&)>, int, int, double, double, double)'
obj/content/renderer/media/content.media_stream_video_source.o: In function
`content::MediaStreamVideoSource::OnSupportedFormats(std::vector<media::VideoCaptureFormat,
std::allocator<media::VideoCaptureFormat> > const&)':
media_stream_video_source.cc:(.text._ZN7content22MediaStreamVideoSource18OnSupportedFormatsERKSt6vectorIN5media18VideoCaptureFormatESaIS3_EE+0xe1):
undefined reference to
`content::VideoTrackAdapter::DeliverFrameOnIO(scoped_refptr<media::VideoFrame>
const&, base::TimeTicks const&)'
obj/content/renderer/media/content.media_stream_video_source.o: In function
`content::MediaStreamVideoSource::OnStartDone(content::MediaStreamRequestResult)':
media_stream_video_source.cc:(.text._ZN7content22MediaStreamVideoSource11OnStartDoneENS_24MediaStreamRequestResultE+0x14e):
undefined reference to `content::VideoTrackAdapter::StartFrameMonitoring(double,
base::Callback<void (bool)> const&)'
media_stream_video_source.cc:(.text._ZN7content22MediaStreamVideoSource11OnStartDoneENS_24MediaStreamRequestResultE+0x184):
undefined reference to `content::MediaStreamSource::StopSource()'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.

[miu, 2015-03-04 01:50:45]

On 2015/03/04 01:35:41, Mostyn Bramley-Moore wrote:
> On 2015/03/04 01:28:56, miu wrote:
> > lgtm.  Sorry for the churn caused by my change.
> > 
> > Out of curiosity, why is media_stream_video_source.cc only supposed to be
> > included in WebRTC builds?  Seems like this file contains nothing
> > WebRTC-specific and is relevant to any producers/consumers around the
> > getUserMedia() and desktop/tab capture APIs.  What happens if the .cc file
is
> > included in non-WebRTC builds?
> 
> I tried moving media_stream_video_source.cc to the private_renderer_sources
list
> earlier, but that gives a bunch of other unresolved symbols when webrtc is
> disabled:

OIC.  Looks like a number of things that aren't WebRTC-specific are being built
only when webrtc is enabled in the build.  Looking through the list of
"undefined reference" errors you posted:

1. VideoTrackAdapter, MediaStreamSource, MediaStreamVideoSource: none of the
code seems to be WebRTC-specific.
2. MediaStreamVideoTrack: nothing WebRTC-specific, but the .cc file seems to
include a libjingle header that is not used for anything.  That include could be
deleted.

So, perhaps the .h/.cc files associated with the above classes should just be
moved over to the other build target?  Feel free to either address the problem
in this change, or punt and just add a TODO comment w/ crbug: It looks like
there's some post-refactoring clean-up work the WebRTC owners should address
here.

[Mostyn Bramley-Moore, 2015-03-04 08:36:39]

On 2015/03/04 01:50:45, miu wrote:
> On 2015/03/04 01:35:41, Mostyn Bramley-Moore wrote:
> > On 2015/03/04 01:28:56, miu wrote:
> > > lgtm.  Sorry for the churn caused by my change.
> > > 
> > > Out of curiosity, why is media_stream_video_source.cc only supposed to be
> > > included in WebRTC builds?  Seems like this file contains nothing
> > > WebRTC-specific and is relevant to any producers/consumers around the
> > > getUserMedia() and desktop/tab capture APIs.  What happens if the .cc file
> is
> > > included in non-WebRTC builds?
> > 
> > I tried moving media_stream_video_source.cc to the private_renderer_sources
> list
> > earlier, but that gives a bunch of other unresolved symbols when webrtc is
> > disabled:
> 
> OIC.  Looks like a number of things that aren't WebRTC-specific are being
built
> only when webrtc is enabled in the build.  Looking through the list of
> "undefined reference" errors you posted:
> 
> 1. VideoTrackAdapter, MediaStreamSource, MediaStreamVideoSource: none of the
> code seems to be WebRTC-specific.
> 2. MediaStreamVideoTrack: nothing WebRTC-specific, but the .cc file seems to
> include a libjingle header that is not used for anything.  That include could
be
> deleted.
> 
> So, perhaps the .h/.cc files associated with the above classes should just be
> moved over to the other build target?  Feel free to either address the problem
> in this change, or punt and just add a TODO comment w/ crbug: It looks like
> there's some post-refactoring clean-up work the WebRTC owners should address
> here.

Filed crbug.com/463829 to follow up, but that might take me a couple of days to
get around to so I have added a TODO note and would like to land this temporary
fix in the meantime.

[DaleCurtis, 2015-03-04 19:21:30]

https://codereview.chromium.org/973373002/diff/20001/content/content_renderer...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/973373002/diff/20001/content/content_renderer...
content/content_renderer.gypi:276:
'renderer/media/media_stream_video_source_constants.cc', # TODO(mostynb):
crbug.com/463829
Move comment to line above since it breaks 80 chars.

https://codereview.chromium.org/973373002/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source_constants.cc (right):

https://codereview.chromium.org/973373002/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_video_source_constants.cc:12: const int
MediaStreamVideoSource::kDefaultWidth = 640;
Why not just stick these as enum {} in a shared header? No need to make them
static consts.

[Mostyn Bramley-Moore, 2015-03-04 20:23:47]

https://codereview.chromium.org/973373002/diff/20001/content/content_renderer...
File content/content_renderer.gypi (right):

https://codereview.chromium.org/973373002/diff/20001/content/content_renderer...
content/content_renderer.gypi:276:
'renderer/media/media_stream_video_source_constants.cc', # TODO(mostynb):
crbug.com/463829
On 2015/03/04 19:21:30, DaleCurtis wrote:
> Move comment to line above since it breaks 80 chars.

Removed this file instead.

https://codereview.chromium.org/973373002/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source_constants.cc (right):

https://codereview.chromium.org/973373002/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_video_source_constants.cc:12: const int
MediaStreamVideoSource::kDefaultWidth = 640;
On 2015/03/04 19:21:30, DaleCurtis wrote:
> Why not just stick these as enum {} in a shared header? No need to make them
> static consts.

Actually why not keep them as static consts, but with the values in
media_stream_video_source.h?  (Done in the latest patchset.)

[DaleCurtis, 2015-03-04 21:26:04]

https://codereview.chromium.org/973373002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/973373002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:81: static const int
kDefaultWidth = 640;
Because you don't want static values in a header file, each includer gets a new
copy:

http://stackoverflow.com/questions/92546/variable-declarations-in-header-file...

Which is why I suggested you make them enum values :)

[Mostyn Bramley-Moore, 2015-03-04 21:49:40]

https://codereview.chromium.org/973373002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/973373002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:81: static const int
kDefaultWidth = 640;
On 2015/03/04 21:26:04, DaleCurtis wrote:
> Because you don't want static values in a header file, each includer gets a
new
> copy:

I figured since it was only a few ints (as opposed to eg command line arg
strings) it wasn't so bad.  It builds for me locally but not on GOMA/the try
bots so the point is moot.  

>
http://stackoverflow.com/questions/92546/variable-declarations-in-header-file...
> 
> Which is why I suggested you make them enum values :)

Hmm, but don't you then need to cast to int every time you want to use these
enum values?  That would be annoying if the values are used often.

[DaleCurtis, 2015-03-04 21:54:23]

lgtm

https://codereview.chromium.org/973373002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/973373002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:81: static const int
kDefaultWidth = 640;
On 2015/03/04 21:49:39, Mostyn Bramley-Moore wrote:
> On 2015/03/04 21:26:04, DaleCurtis wrote:
> > Because you don't want static values in a header file, each includer gets a
> new
> > copy:
> 
> I figured since it was only a few ints (as opposed to eg command line arg
> strings) it wasn't so bad.  It builds for me locally but not on GOMA/the try
> bots so the point is moot.  
> 
> >
>
http://stackoverflow.com/questions/92546/variable-declarations-in-header-file...
> > 
> > Which is why I suggested you make them enum values :)
> 
> Hmm, but don't you then need to cast to int every time you want to use these
> enum values?  That would be annoying if the values are used often.

It depends on how they're used; in the majority of cases it'll be an implicit
conversion without issue.

[DaleCurtis, 2015-03-04 21:54:25]

lgtm

lgtm

[Mostyn Bramley-Moore, 2015-03-04 21:57:39]

https://codereview.chromium.org/973373002/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_video_source.h (right):

https://codereview.chromium.org/973373002/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_video_source.h:81: static const int
kDefaultWidth = 640;
On 2015/03/04 21:54:23, DaleCurtis wrote:
> On 2015/03/04 21:49:39, Mostyn Bramley-Moore wrote:
> > On 2015/03/04 21:26:04, DaleCurtis wrote:
> > > Because you don't want static values in a header file, each includer gets
a
> > new
> > > copy:
> > 
> > I figured since it was only a few ints (as opposed to eg command line arg
> > strings) it wasn't so bad.  It builds for me locally but not on GOMA/the try
> > bots so the point is moot.  
> > 
> > >
> >
>
http://stackoverflow.com/questions/92546/variable-declarations-in-header-file...
> > > 
> > > Which is why I suggested you make them enum values :)
> > 
> > Hmm, but don't you then need to cast to int every time you want to use these
> > enum values?  That would be annoying if the values are used often.
> 
> It depends on how they're used; in the majority of cases it'll be an implicit
> conversion without issue.

OK.  I will try to eliminate the need for this later in the week as part of
crbug.com/463829.

[Mostyn Bramley-Moore, 2015-03-04 22:22:43]

The CQ bit was checked by mostynb@opera.com

[Mostyn Bramley-Moore, 2015-03-04 22:22:43]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/973373002/#ps60001 (title:
"convert int constants to enum values")

[commit-bot: I haz the power, 2015-03-04 22:23:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/973373002/60001

[commit-bot: I haz the power, 2015-03-05 00:14:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-05 00:15:37]

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