Remove VideoDecoder::natural_size() & added VideoFrame::natural_size().

Remove VideoDecoder::natural_size() & added VideoFrame::natural_size().


BUG=122913
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=149728

[acolwell GONE FROM CHROMIUM, 2012-08-01 23:58:13]

http://codereview.chromium.org/10824141/diff/1/media/filters/ffmpeg_video_dec...
File media/filters/ffmpeg_video_decoder.h (left):

http://codereview.chromium.org/10824141/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder.h:112: int frame_rate_denominator_;
Missed these on my last CL.

http://codereview.chromium.org/10824141/diff/1/media/filters/video_renderer_b...
File media/filters/video_renderer_base_unittest.cc (right):

http://codereview.chromium.org/10824141/diff/1/media/filters/video_renderer_b...
media/filters/video_renderer_base_unittest.cc:117: EXPECT_CALL(*this,
OnNaturalSizeChanged(kNaturalSize));
This moves here because the notification happens when VideoRendererBase request
the paint for the first frame now.

[Ami GONE FROM CHROMIUM, 2012-08-02 00:16:14]

http://codereview.chromium.org/10824141/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

http://codereview.chromium.org/10824141/diff/1/media/base/video_frame.h#newco...
media/base/video_frame.h:45: size_t width,
this width/height pair should be gfx::Size'ified.  
And probably named coded_size.

It would be great if relevant portions of vrk@'s doc could be migrated to
comments here or nearby.

http://codereview.chromium.org/10824141/diff/1/media/base/video_frame.h#newco...
media/base/video_frame.h:139: // Width and height of surface.
"surface" is particularly unenlightening in this context, IMO.

http://codereview.chromium.org/10824141/diff/1/media/filters/ffmpeg_video_dec...
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/10824141/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder.h:109: // Natural size for the last
VideoDecoderConfig received.
Why does this get to live?

http://codereview.chromium.org/10824141/diff/1/media/filters/gpu_video_decode...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/10824141/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.cc:41: : bitstream_buffer_id(bbid),
timestamp(ts), natural_size(natural_size) {
natural_size is a self-assignment here.

http://codereview.chromium.org/10824141/diff/1/media/filters/gpu_video_decoder.h
File media/filters/gpu_video_decoder.h (right):

http://codereview.chromium.org/10824141/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.h:145: // Natural size from the last
VideoDecoderConfig received.
Ditto

http://codereview.chromium.org/10824141/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.h:198: const gfx::Size& natural_size);
indent is off

http://codereview.chromium.org/10824141/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.h:204: std::list<BufferData>
input_buffer_time_data_;
s/_time//

[acolwell GONE FROM CHROMIUM, 2012-08-02 02:16:29]

PTAL.. I just want to make sure this is going in a direction you're happy with.

http://codereview.chromium.org/10824141/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

http://codereview.chromium.org/10824141/diff/1/media/base/video_frame.h#newco...
media/base/video_frame.h:45: size_t width,
On 2012/08/02 00:16:14, Ami Fischman wrote:
> this width/height pair should be gfx::Size'ified.  
> And probably named coded_size.
Done, but named size since it technically is the visible size instead of the
coded size. I thought the visible_ prefix might be confusing so I left it off.
WDYT?

> 
> It would be great if relevant portions of vrk@'s doc could be migrated to
> comments here or nearby.
I'll do this in my next patch update.

http://codereview.chromium.org/10824141/diff/1/media/filters/ffmpeg_video_dec...
File media/filters/ffmpeg_video_decoder.h (right):

http://codereview.chromium.org/10824141/diff/1/media/filters/ffmpeg_video_dec...
media/filters/ffmpeg_video_decoder.h:109: // Natural size for the last
VideoDecoderConfig received.
On 2012/08/02 00:16:14, Ami Fischman wrote:
> Why does this get to live?
Removed.

http://codereview.chromium.org/10824141/diff/1/media/filters/gpu_video_decode...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/10824141/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.cc:41: : bitstream_buffer_id(bbid),
timestamp(ts), natural_size(natural_size) {
On 2012/08/02 00:16:14, Ami Fischman wrote:
> natural_size is a self-assignment here.
oops.. Done

http://codereview.chromium.org/10824141/diff/1/media/filters/gpu_video_decoder.h
File media/filters/gpu_video_decoder.h (right):

http://codereview.chromium.org/10824141/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.h:145: // Natural size from the last
VideoDecoderConfig received.
On 2012/08/02 00:16:14, Ami Fischman wrote:
> Ditto
Gone.

http://codereview.chromium.org/10824141/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.h:198: const gfx::Size& natural_size);
On 2012/08/02 00:16:14, Ami Fischman wrote:
> indent is off

Done.

http://codereview.chromium.org/10824141/diff/1/media/filters/gpu_video_decode...
media/filters/gpu_video_decoder.h:204: std::list<BufferData>
input_buffer_time_data_;
On 2012/08/02 00:16:14, Ami Fischman wrote:
> s/_time//

Done.

[acolwell GONE FROM CHROMIUM, 2012-08-02 16:46:22]

Ok now PTAL.. for realz

[Ami GONE FROM CHROMIUM, 2012-08-02 17:34:44]

LGTM % nits.

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.cc
File media/base/video_frame.cc (right):

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.cc#...
media/base/video_frame.cc:44: const gfx::Size& size) {
shouldn't this now also inspect natural_size?

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.h
File media/base/video_frame.h (right):

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.h#n...
media/base/video_frame.h:43: // |size| is the width and height of the frame data
in pixels.
This line does not tell me this is "coded size".  Can we just name it
coded_size, instead?

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.h#n...
media/base/video_frame.h:59: // |size| is the width and height of the frame data
in pixels.
ditto

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.h#n...
media/base/video_frame.h:80: const gfx::Size& size() const { return size_; }
ditto

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.h#n...
media/base/video_frame.h:121: const gfx::Size& size,
ditto

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.h#n...
media/base/video_frame.h:136: // Width and height of the video frame.
ditto

http://codereview.chromium.org/10824141/diff/12003/media/filters/gpu_video_de...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/10824141/diff/12003/media/filters/gpu_video_de...
media/filters/gpu_video_decoder.cc:235: // |status| == kConfigChanged. For now
we just trigger a decode error.
Do we still need this?
IOW if the decoders don't maintain config-related state, and instead query the
demuxer/stream as needed, can kConfigChanged die?

[acolwell GONE FROM CHROMIUM, 2012-08-02 20:20:20]

address comments & fixed WebRTC content_unittests

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.cc
File media/base/video_frame.cc (right):

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.cc#...
media/base/video_frame.cc:44: const gfx::Size& size) {
On 2012/08/02 17:34:45, Ami Fischman wrote:
> shouldn't this now also inspect natural_size?

Done.

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.h
File media/base/video_frame.h (right):

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.h#n...
media/base/video_frame.h:43: // |size| is the width and height of the frame data
in pixels.
On 2012/08/02 17:34:45, Ami Fischman wrote:
> This line does not tell me this is "coded size".  Can we just name it
> coded_size, instead?
Renamed to data_size as agreed in offline discussion.

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.h#n...
media/base/video_frame.h:59: // |size| is the width and height of the frame data
in pixels.
On 2012/08/02 17:34:45, Ami Fischman wrote:
> ditto

Done.

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.h#n...
media/base/video_frame.h:80: const gfx::Size& size() const { return size_; }
On 2012/08/02 17:34:45, Ami Fischman wrote:
> ditto

Done.

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.h#n...
media/base/video_frame.h:121: const gfx::Size& size,
On 2012/08/02 17:34:45, Ami Fischman wrote:
> ditto

Done.

http://codereview.chromium.org/10824141/diff/12003/media/base/video_frame.h#n...
media/base/video_frame.h:136: // Width and height of the video frame.
On 2012/08/02 17:34:45, Ami Fischman wrote:
> ditto

Done.

http://codereview.chromium.org/10824141/diff/12003/media/filters/gpu_video_de...
File media/filters/gpu_video_decoder.cc (right):

http://codereview.chromium.org/10824141/diff/12003/media/filters/gpu_video_de...
media/filters/gpu_video_decoder.cc:235: // |status| == kConfigChanged. For now
we just trigger a decode error.
On 2012/08/02 17:34:45, Ami Fischman wrote:
> Do we still need this?
> IOW if the decoders don't maintain config-related state, and instead query the
> demuxer/stream as needed, can kConfigChanged die?
No. We still need this because the decoders hold state inside them when they are
initialized. This status is needed so that the decoders know when they need to
reinitialize this internal state.

[Ami GONE FROM CHROMIUM, 2012-08-02 21:41:31]

LGTM

http://codereview.chromium.org/10824141/diff/5006/content/renderer/media/capt...
File content/renderer/media/capture_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10824141/diff/5006/content/renderer/media/capt...
content/renderer/media/capture_video_decoder_unittest.cc:179:
TEST_F(CaptureVideoDecoderTest, ReadAndResetonde) {
onde?

[acolwell GONE FROM CHROMIUM, 2012-08-02 22:38:18]

http://codereview.chromium.org/10824141/diff/5006/content/renderer/media/capt...
File content/renderer/media/capture_video_decoder_unittest.cc (right):

http://codereview.chromium.org/10824141/diff/5006/content/renderer/media/capt...
content/renderer/media/capture_video_decoder_unittest.cc:179:
TEST_F(CaptureVideoDecoderTest, ReadAndResetonde) {
On 2012/08/02 21:41:32, Ami Fischman wrote:
> onde?
Oops.. not sure what happened there. Removed.