Issue 13890012: Integrate VDA with WebRTC.

wuchengli

Pawel and Ami. I've updated Dongwon's CL and uploaded it. apprtc loopback is still not ...

7 years, 8 months ago (2013-04-25 12:35:40 UTC) #1

wuchengli

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_video_decoder_bridge.cc File content/renderer/media/rtc_video_decoder_bridge.cc (right): https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_video_decoder_bridge.cc#newcode317 content/renderer/media/rtc_video_decoder_bridge.cc:317: ready_video_frames_.push_back(frame); How can I call decode_complete_callback_ using webrtc DecodingThread ...

7 years, 8 months ago (2013-04-25 12:38:54 UTC) #2

Ami GONE FROM CHROMIUM

See my comment at content/renderer/media/rtc_video_decoder_bridge.cc:203 for what I believe your problem is and how to ...

7 years, 8 months ago (2013-04-26 00:42:04 UTC) #3

wuchengli

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_video_decoder_bridge.cc File content/renderer/media/rtc_video_decoder_bridge.cc (right): https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_video_decoder_bridge.cc#newcode203 content/renderer/media/rtc_video_decoder_bridge.cc:203: scoped_refptr<media::DecoderBuffer> buffer; GpuVideoDecoder::Read is called in RTCVideoDecoderBridge::RequestFrame. I think ...

7 years, 8 months ago (2013-04-26 05:08:08 UTC) #4

Pawel Osciak

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_video_decoder_bridge.cc File content/renderer/media/rtc_video_decoder_bridge.cc (right): https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_video_decoder_bridge.cc#newcode118 content/renderer/media/rtc_video_decoder_bridge.cc:118: if (codecSettings->codecType != webrtc::kVideoCodecVP8) { Can this happen if ...

7 years, 8 months ago (2013-04-26 07:08:01 UTC) #5

wuchengli

I've addressed some review comments. I'll address the rest after I come back from the ...

7 years, 8 months ago (2013-04-26 11:49:14 UTC) #6

Ami GONE FROM CHROMIUM

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_video_decoder_bridge.cc File content/renderer/media/rtc_video_decoder_bridge.cc (right): https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_video_decoder_bridge.cc#newcode203 content/renderer/media/rtc_video_decoder_bridge.cc:203: scoped_refptr<media::DecoderBuffer> buffer; On 2013/04/26 05:08:08, wuchengli wrote: > GpuVideoDecoder::Read ...

7 years, 8 months ago (2013-04-26 15:13:33 UTC) #7

wuchengli

I'm not sure how to use GVDAH. Where in the source code can I take ...

7 years, 8 months ago (2013-04-26 15:38:55 UTC) #8

Ami GONE FROM CHROMIUM

On Fri, Apr 26, 2013 at 8:38 AM, <wuchengli@chromium.org> wrote: > I'm not sure how ...

7 years, 8 months ago (2013-04-26 16:07:06 UTC) #9

Ami GONE FROM CHROMIUM

On Fri, Apr 26, 2013 at 4:49 AM, <wuchengli@chromium.org> wrote: > I've addressed some review ...

7 years, 7 months ago (2013-04-29 18:43:28 UTC) #10

On Fri, Apr 26, 2013 at 4:49 AM, <wuchengli@chromium.org> wrote:

> I've addressed some review comments. I'll address the rest after I come
> back from the vacation.


Ok; let me know when I should look at this again (removing from my TODO
list until then).


> https://codereview.chromium.**org/13890012/diff/1/content/**
>
browser/renderer_host/render_**process_host_impl.cc<https://codereview.chromium.org/13890012/diff/1/content/browser/renderer_host/render_process_host_impl.cc>
> File content/browser/renderer_host/**render_process_host_impl.cc (right):
> https://codereview.chromium.**org/13890012/diff/1/content/**
>
browser/renderer_host/render_**process_host_impl.cc#newcode1<https://codereview.chromium.org/13890012/diff/1/content/browser/renderer_host/render_process_host_impl.cc#newcode1>
> content/browser/renderer_host/**render_process_host_impl.cc:1: //
> Copyright (c) 2012 The Chromium Authors. All rights reserved.
> I added the WebRTC CL, though the link may need to change after I
> correct the base URL. Libjingle is in p10. Can we put internal url in
> the CL description?


It's fine to point to cr/<NNN>-p10 or s10/<NNN> for in-review / submitted
CLs.

https://codereview.chromium.**org/13890012/diff/1/content/**
>
browser/renderer_host/render_**process_host_impl.cc#newcode2<https://codereview.chromium.org/13890012/diff/1/content/browser/renderer_host/render_process_host_impl.cc#newcode2>
> content/browser/renderer_host/**render_process_host_impl.cc:2: // Use of
> this source code is governed by a BSD-style license that can be
> I pasted the log at
https://p.prom.corp.google.**com/2594062<https://p.prom.corp.google.com/2594062>.
> The decoder
> was running. Sometimes loopback started. But overall it was sluggish
> intermittently.


Per subsequent email, I'm pretty sure this is caused by failure to pipeline
the decode calls to the HW.
(should be cured by removing GVD from the picture and using the VDA
interface provided by GVDAH directly)


> https://codereview.chromium.**org/13890012/diff/1/content/**
>
browser/renderer_host/render_**process_host_impl.cc#**newcode832<https://codereview.chromium.org/13890012/diff/1/content/browser/renderer_host/render_process_host_impl.cc#newcode832>
> content/browser/renderer_host/**render_process_host_impl.cc:**832:
> switches::**kEnableWebRTCHWDecoding,
> On 2013/04/26 00:42:05, Ami Fischman wrote:
>
>> This is a strange place to drop this particular flag :)
>>
> I moved it after switches::**kEnableVsyncNotification. Some items in this
> array does not follow alphabetical order... Let me know if you think
> there's a better place. I assume you don't mean to move this to another
> file.


Yeah, that's fine.


> https://codereview.chromium.**org/13890012/diff/1/content/**
>
renderer/media/media_stream_**dependency_factory.cc<https://codereview.chromium.org/13890012/diff/1/content/renderer/media/media_stream_dependency_factory.cc>
> File content/renderer/media/media_**stream_dependency_factory.cc (right):
>
> https://codereview.chromium.**org/13890012/diff/1/content/**
>
renderer/media/media_stream_**dependency_factory.cc#**newcode209<https://codereview.chromium.org/13890012/diff/1/content/renderer/media/media_stream_dependency_factory.cc#newcode209>
> content/renderer/media/media_**stream_dependency_factory.cc:**209:
> gpu_factories_(gpu_factories),
> On 2013/04/26 00:42:05, Ami Fischman wrote:
>
>> DCHECK this is NULL iff the cmd-line flag is missing?
>>
> I changed the place where MediaStreamDependencyFactory is created. If
> gpu_factories is non NULL, kEnableWebRTCHWDecoding is enabled.


Right - I'm just suggesting to DCHECK this invariant in the ctor to ensure
callers don't start doing this wrong.
(alternatively, you could stop inspecting the value of the commandline
switch in MSDF entirely, instead using the presence/absence of
gpu_factories_ as your signal; is there a reason that's a bad idea?)


> https://codereview.chromium.**org/13890012/diff/1/content/**
>
renderer/render_thread_impl.**cc#newcode1240<https://codereview.chromium.org/13890012/diff/1/content/renderer/render_thread_impl.cc#newcode1240>
> content/renderer/render_**thread_impl.cc:1240: if
> (!cmd_line->HasSwitch(**switches::**kDisableAcceleratedVideoDecode**))
> I added a line here. Also, I could not find a good way to extract it in
> RenderViewImpl


Not sure why this can't work as:
RVI::GetDecodeGpuFactoriesForVideo()
and
RVI::GetDecodeGpuFactoriesForWebRTC()
and have the latter call the former if the commandline switch is present?


> because EnsureMediaStreamImpl is called by two places there.


But EMSI() could call GDGFFW(), no?
(or GDGFFW() could simply be inlined into EMSI)

wuchengli

I've addressed most of the review comments. You can take another look. Here are the ...

7 years, 7 months ago (2013-05-08 15:58:56 UTC) #11

I've addressed most of the review comments. You can take another look.

Here are the parts I'm still working on. Please ignore them for now.
- Usage of Unretained in rtc_video_decoder.cc.
- A few review comments that I have not replied.
- rtc_video_decoder is still using GVD instead of VDA.

As I mentioned in the email, currently loopback runs much more smooth after
rebase. The performance looks similar to SW decoder.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/media_...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:215: if
(!decoder_worker_thread_.Start()) {
Sounds good. I've used gpu_factories_ to decide whether to create
RTCVideoDecoderFactory.
On 2013/04/26 11:49:14, wuchengli wrote:
> On 2013/04/26 00:42:05, Ami Fischman wrote:
> > Shouldn't this be conditional on wanting to use GVD?
> I moved this to EnsurePeerConnectionFactory and started the thread only when
> gpu_factories is non-null.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_dependency_factory.cc:451: audio_device_ =
NULL;
No need to delete the decoder_factory because CreatePeerConnectionFactory takes
the ownership and will delete it in all cases. CreatePeerConnectionFactory takes
a pointer so I changed it to decoder_factory.release().
On 2013/04/26 00:42:05, Ami Fischman wrote:
> need to delete the decoder_factory in this case?
> (generally, a useful idiom for this is to place the new'd object directly into
a
> scoped_ptr<> and pass it to CreatePeerConnectionFactory() as
> decoder_factory.Pass()

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
File content/renderer/media/rtc_video_decoder_bridge.cc (right):

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:23: #define LOG_LINE()
VLOG(0) << __FILE__ << ":" << __FUNCTION__
Removed.
On 2013/04/26 00:42:05, Ami Fischman wrote:
> Planning to remove before landing?

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:26: const base::TimeDelta
RTCVideoDecoderBridge::kDecoderTimeOut =
Removed. Now InitDecode waits for GpuVideoDecoder::Initialize to finish.
On 2013/04/26 00:42:05, Ami Fischman wrote:
> Why is this a good idea?

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:30: static void
RunOnMessageLoop(
On 2013/04/26 00:42:05, Ami Fischman wrote:
> Any reason this can't be replaced by BindToLoop?
Removed.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:56: CHECK(false) << "Does not
support audio.";
On 2013/04/26 00:42:05, Ami Fischman wrote:
> FWIW 
> LOG(FATAL)
> reads slightly more clearly to me than
> CHECK(false)

Done.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:57: return
dummy_audio_decoder_config_;
On 2013/04/26 00:42:05, Ami Fischman wrote:
> this is unreachable, and therefore the member is unnecessary.
Removed.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:69: }
On 2013/04/26 00:42:05, Ami Fischman wrote:
> LOG(FATAL) << "Unreachable!";
Changed to NOTREACHED();

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:77: size, rect, size, NULL,
0, false, false);
On 2013/04/26 00:42:05, Ami Fischman wrote:
> Coded==visible==natural sizes/rects?
> I suspect there's an impedance mismatch between media::VF and webrtc that at
> least warrants a comment.
Can you explain more? I'm not sure what to put in the comment. I cannot find
information about visible rect or pixel aspect ratio in webrtc.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:118: if
(codecSettings->codecType != webrtc::kVideoCodecVP8) {
On 2013/04/26 07:08:01, posciak wrote:
> Can this happen if the check in RTCVideoDecoderFactory::CreateVideoDecoder()
> doesn't fail? Perhaps we could drop it and keep this one?
> 
> Or actually, ideally, let VideoDecoder::Initialize decide.
All possible codecType are kVideoCodecVP8, kVideoCodecI420, kVideoCodecRED,
kVideoCodecULPFEC, kVideoCodecGeneric, and kVideoCodecUnknown. In
RTCVideoDecoderFactory::CreateVideoDecoder, we don't have
video_codec_config.size. So we cannot call VideoDecoder::Initialize there.

We can let VideoDecoder::Initialize decide. A mapping table will be needed.
Since only VP8 is the only option that can be mapped to
media::VideoCodecProfile. We don't need to add the code not used now. Thought?

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:142: }
On 2013/04/26 07:08:01, posciak wrote:
> No need for braces.
I added one more line.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:171: WebRtc_Word64
renderTimeMs) {
On 2013/04/26 00:42:05, Ami Fischman wrote:
> unused parameters can have their name omitted, which makes it more obvious
that
> they are unused.  Please do that for unused params.

Done.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:174: base::AutoLock
auto_lock(lock_);
On 2013/04/26 07:08:01, posciak wrote:
> Lock usage is mysterious here, especially since there is a return after this
and
> no change to decoding_error_occured_.
decoding_error_occurred_ is read by WebRTC DecodingThread and set by
decoder_message_loop_. I added comments in the header file and used volatile
instead.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:176: // TODO(dwkang):
consider creating a SW decoder and falling back to it.
On 2013/04/26 00:42:05, Ami Fischman wrote:
> s/dwkang/wuchengli/g ? :)
Removed the todo. This doesn't look like the right place for SW decoder.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:182: }
On 2013/04/26 07:08:01, posciak wrote:
> Braces not needed and below too.

Done.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:183: if (!(status_ ==
kInitialized || status_ == kDecoding)) {
On 2013/04/26 07:08:01, posciak wrote:
> if (status_ != kInitialized && status_ != kDecoding)

Done.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:197: size_ = new_size;
On 2013/04/26 00:42:05, Ami Fischman wrote:
> member never used; drop?
This is used so that RTCDemuxerStream::UpdateSize is not called when the size is
unchanged.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:204: buffer =
media::DecoderBuffer::CopyFrom(
On 2013/04/26 00:42:05, Ami Fischman wrote:
> assign on prev line?

Done.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:280: base::WaitableEvent
event(false, false);
On 2013/04/26 07:08:01, posciak wrote:
> unused?
Removed.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
File content/renderer/media/rtc_video_decoder_bridge.h (right):

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.h:26: class RTCDemuxerStream :
public media::DemuxerStream {
Done. Moved to .cc file.
On 2013/04/26 00:42:05, Ami Fischman wrote:
> Any reason this can't live entirely in the .cc file (with just a fwd
declaration
> here)?

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.h:39: friend class
base::RefCountedThreadSafe<RTCDemuxerStream>;
Removed.
On 2013/04/26 00:42:05, Ami Fischman wrote:
> DemuxerStream shouldn't be refcounted anymore so you should be able to have
more
> explicit ownership semantics for the stream (yay)
>
http://src.chromium.org/viewvc/chrome/trunk/src/media/base/demuxer_stream.h?r...

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.h:41: private:
The constructor and other methods of RTCDemuxerStream are called in different
threads. I added a message loop proxy to make sure the methods are called on the
same thread.
On 2013/04/26 00:42:05, Ami Fischman wrote:
> Would it make sense to give this a ThreadChecker member that asserts this
class
> is always called on the same thread?

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.h:49: class CONTENT_EXPORT
RTCVideoDecoderBridge
On 2013/04/26 07:08:01, posciak wrote:
> Let's rename this to RTCVideoDecoder, we don't have the old RVD anymore
> (crbug.com/177572).
Done. Renamed to RTCVideoDecoder.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.h:75: enum Status {
Done.
On 2013/04/26 07:08:01, posciak wrote:
> s/Status/State maybe?

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.h:76: kNoInit,
On 2013/04/26 07:08:01, posciak wrote:
> s/kNoInit/kUninitialized ?

Done.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.h:80: kReleased,
On 2013/04/26 07:08:01, posciak wrote:
> Not needed, use kNoInit/kUninitialized instead?

Done.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.h:83:
scoped_refptr<media::VideoDecoder> video_decoder_;
Rebased.
On 2013/04/26 00:42:05, Ami Fischman wrote:
> I suspect you need to rebase since this is no longer refcounted (r195212)

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.h:92: webrtc::I420VideoFrame
decoded_image_;
Removed.
On 2013/04/26 07:08:01, posciak wrote:
> Does this need to be a class member?

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.h:99: void
OnUpdateStatistics(const media::PipelineStatistics& stats);
On 2013/04/26 00:42:05, Ami Fischman wrote:
> nit: methods go before members

Done.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
File content/renderer/media/rtc_video_decoder_factory.cc (right):

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_factory.cc:12: :
chrome_loop_proxy_(chrome_loop_proxy),
On 2013/04/26 00:42:05, Ami Fischman wrote:
> nit: +2 additional indent this and the next line

Done.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_factory.cc:19: if (type ==
webrtc::kVideoCodecVP8) {
H264 is not in the enum. All enum values are kVideoCodecVP8, kVideoCodecI420,
kVideoCodecRED, kVideoCodecULPFEC, kVideoCodecGeneric, and kVideoCodecUnknown.

It looks like libjingle expects this method to return null if the codec is not
supported. Maybe we should call GpuVideoDecoder::Initialize here. However,
GpuVideoDecoder::Initialize has a DHECK about VideoDecoderConfig.size != 0. We
don't have size information here.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

On 2013/04/26 07:08:01, posciak wrote:
> On 2013/04/26 00:42:05, Ami Fischman wrote:
> > why not also allow h264 here?
> 
> This should be decided by each vda implementation ideally...

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
File content/renderer/media/rtc_video_decoder_factory.h (right):

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_factory.h:26: const
scoped_refptr<base::MessageLoopProxy>& chrome_loop_proxy,
Changed to decoder_message_loop_.
On 2013/04/26 00:42:05, Ami Fischman wrote:
> this isn't a useful variable name

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_factory.h:35:
scoped_refptr<base::MessageLoopProxy> chrome_loop_proxy_;
Changed to decoder_message_loop_.
On 2013/04/26 00:42:05, Ami Fischman wrote:
> ditto naming

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
File content/renderer/media/rtc_video_renderer.cc (right):

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_renderer.cc:88: (webrtc::NativeHandle
*)frame->GetNativeHandle();
I don't understand. The Impl version signature also returns void*. The cast is
still needed.
On 2013/04/26 00:42:05, Ami Fischman wrote:
> if you cast this to the Impl version then you can avoid the cast to VideoFrame
> in the next call

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_renderer.cc:90: base::TimeDelta timestamp =
base::TimeDelta::FromMilliseconds(
Done. Someone else extracted this.
On 2013/04/26 00:42:05, Ami Fischman wrote:
> this is common to both if/else clauses so could be pulled out.

https://codereview.chromium.org/13890012/diff/1/content/renderer/render_threa...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/13890012/diff/1/content/renderer/render_threa...
content/renderer/render_thread_impl.cc:1240: if
(!cmd_line->HasSwitch(switches::kDisableAcceleratedVideoDecode))
GetMediaStreamDependencyFactory is also called by other methods in
RenderThreadImpl. So gpu_factories cannot be passed from from RenderViewImpl. I
extracted this to a new method in RenderThreadImpl and let RenderViewImpl call
it.
On 2013/04/26 11:49:14, wuchengli wrote:
> I added a line here. Also, I could not find a good way to extract it in
> RenderViewImpl because EnsureMediaStreamImpl is called by two places there.
> On 2013/04/26 00:42:05, Ami Fischman wrote:
> > IWBN to avoid the code duplication between here and
> > RenderViewImpl::createMediaPlayer().  Does it make sense to extract to a
> helper
> > in RVI and have RVI pass the (possibly-NULL) |gpu_factories| to this method?
>

https://codereview.chromium.org/13890012/diff/1/webkit/media/webmediaplayer_m...
File webkit/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/13890012/diff/1/webkit/media/webmediaplayer_m...
webkit/media/webmediaplayer_ms.cc:437: //
media::VideoFrame::CreateBlackFrame(frame->natural_size());
As discussed in email, |current_frame| is simply set to |frame|.
On 2013/04/26 00:42:05, Ami Fischman wrote:
> I'm not sure what this is about

wuchengli

This is ready for review. I've addressed all review comments. https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_video_decoder_bridge.cc File content/renderer/media/rtc_video_decoder_bridge.cc (right): https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_video_decoder_bridge.cc#newcode203 ...

7 years, 7 months ago (2013-05-09 15:43:14 UTC) #12

This is ready for review. I've addressed all review comments.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
File content/renderer/media/rtc_video_decoder_bridge.cc (right):

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:203:
scoped_refptr<media::DecoderBuffer> buffer;
On 2013/04/26 15:13:34, Ami Fischman wrote:
> On 2013/04/26 05:08:08, wuchengli wrote:
> > GpuVideoDecoder::Read is called in RTCVideoDecoderBridge::RequestFrame. I
> think
> > it triggers GpuVideoDecoder decoding. From the log, the decode did start.
> > https://p.prom.corp.google.com/2594062  It was just sluggish intermittently.

> 
> I see.  IIUC the code will wait for each frame to round-trip from the GPU
> process before requesting the next frame (and thus sending the bitstream to
the
> GPU process) so sluggishness is not at all surprising (instead, you want to
> pipeline the decodes).  I still think GVD is doing you a disservice here and
> that you should be using VDA directly (via the concrete impl of GVDAH, which
is
> what GVD uses).
The latest patchset does not wait from the GPU process before requesting the
next frame. RTCVideoDecoder::Decode is run by webrtc DecodingThread and keeps
queuing buffers. FrameReady is run by decoder_message_loop_ (same as
gvd_loop_proxy_ in GpuVideoDecoder and is a new thread created in
MediaStreamDependencyFactory).

One question is whether it is safe for webrtc to call decode complete callback
from the other thread. video_codec_interface.h doesn't mention this.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.cc:207: inputImage._timeStamp));
On 2013/04/26 00:42:05, Ami Fischman wrote:
> This looks strange; internalvalue is in microseconds but _timeStamp is an
int32
> so likely not.
inputImage._timeStamp is in 90KHz. We need to divide it by 90 here and multiply
it by 90 when getting the frame from decoder in FrameReady(). It may lose the
precision. However, webrtc uses the timestamp as key of a map. I cannot think of
a good solution. Adding another map here is not good, either. Multiple 90Khz
timestamps can be converted to the same timestamp in microsecond. Any
suggestion?

Use timestamp as key to get other data from the map:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

Other webrtc::VideoDecoder::Decoder implementation are blocking. They return the
decoded frame and can easily use the same timestamp.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
File content/renderer/media/rtc_video_decoder_bridge.h (right):

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.h:49: class CONTENT_EXPORT
RTCVideoDecoderBridge
On 2013/04/26 00:42:05, Ami Fischman wrote:
> Could benefit from commentary saying where this fits into the world and what
its
> threading story is (where it is called from, where it dispatches to)
Done. I added some comments.

https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge.h:83:
scoped_refptr<media::VideoDecoder> video_decoder_;
On 2013/04/26 00:42:05, Ami Fischman wrote:
> Members whose use/purpose is non-obvious should be commented.
Comment added for member variables.

wuchengli

https://codereview.chromium.org/13890012/diff/50001/content/renderer/media/rtc_video_decoder.cc File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/13890012/diff/50001/content/renderer/media/rtc_video_decoder.cc#newcode213 content/renderer/media/rtc_video_decoder.cc:213: scoped_refptr<media::DecoderBuffer> buffer = media::DecoderBuffer::CopyFrom( Copy is necessary because the ...

7 years, 7 months ago (2013-05-10 15:57:15 UTC) #13

Ami GONE FROM CHROMIUM

Not reviewing code (per off-reviewlog convo, waiting for some more changes to filter in), only ...

7 years, 7 months ago (2013-05-13 04:03:33 UTC) #14

Not reviewing code (per off-reviewlog convo, waiting for some more changes
to filter in), only addressing q's in previous responses below.

One question is whether it is safe for webrtc to call decode complete
> callback from the other thread. video_codec_interface.h doesn't
> mention this.
>

Ask webrtc-eng or trace the call graph and see if the result is trampolined
anyway (I'm not sure what the answer is off-hand).


> https://codereview.chromium.**org/13890012/diff/1/content/**
>
renderer/media/rtc_video_**decoder_bridge.cc#newcode207<https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_video_decoder_bridge.cc#newcode207>
> content/renderer/media/rtc_**video_decoder_bridge.cc:207:
> inputImage._timeStamp));
>
> On 2013/04/26 00:42:05, Ami Fischman wrote:
>
>> This looks strange; internalvalue is in microseconds but _timeStamp is
>>
> an int32
>
>> so likely not.
>>
> inputImage._timeStamp is in 90KHz. We need to divide it by 90 here and
> multiply it by 90 when getting the frame from decoder in FrameReady().
> It may lose the precision. However, webrtc uses the timestamp as key of
> a map. I cannot think of a good solution. Adding another map here is not
> good, either. Multiple 90Khz timestamps can be converted to the same
> timestamp in microsecond. Any suggestion?
>
> Use timestamp as key to get other data from the map:
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/third_party/webrtc/**modules/video_coding/main/**
> source/generic_decoder.cc&q=**generic_decoder.cc&sq=package:**
>
chromium&l=49<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc/modules/video_coding/main/source/generic_decoder.cc&q=generic_decoder.cc&sq=package:chromium&l=49>
>
> Other webrtc::VideoDecoder::Decoder implementation are blocking. They
> return the decoded frame and can easily use the same timestamp.
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/third_party/webrtc/**modules/video_coding/codecs/**
>
vp8/vp8_impl.cc&sq=package:**chromium&l=699<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc&sq=package:chromium&l=699>
>
>
OIC, you're just using the TimeDelta as a holder.  That's fine (a hack, but
fine, since you're bridging inter-API dissonance) as long as you add a
comment explaining what you're doing to both in and out locations.


> https://codereview.chromium.**org/13890012/diff/1/content/**
>
renderer/media/rtc_video_**decoder_bridge.cc#newcode77<https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_video_decoder_bridge.cc#newcode77>
> content/renderer/media/rtc_**video_decoder_bridge.cc:77: size, rect, size,
> NULL, 0, false, false);
> On 2013/04/26 00:42:05, Ami Fischman wrote:
>
>> Coded==visible==natural sizes/rects?
>> I suspect there's an impedance mismatch between media::VF and webrtc
>>
> that at
>
>> least warrants a comment.
>>
> Can you explain more? I'm not sure what to put in the comment. I cannot
> find information about visible rect or pixel aspect ratio in webrtc.
>

I420VideoFrame has the notion of stride for each plane, separate from width
(affecting padding/cropping).
Seemingly there is no support for visible rect other than
(0,0,width,height) and aspect ratio other than 1:1.


> https://codereview.chromium.**org/13890012/diff/1/content/**
>
renderer/media/rtc_video_**decoder_bridge.cc#newcode118<https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_video_decoder_bridge.cc#newcode118>
> content/renderer/media/rtc_**video_decoder_bridge.cc:118: if
> (codecSettings->codecType != webrtc::kVideoCodecVP8) {
> On 2013/04/26 07:08:01, posciak wrote:
>
>> Can this happen if the check in
>>
> RTCVideoDecoderFactory::**CreateVideoDecoder()
>
>> doesn't fail? Perhaps we could drop it and keep this one?
>>
>
> Or actually, ideally, let VideoDecoder::Initialize decide.
>>
> All possible codecType are kVideoCodecVP8, kVideoCodecI420,
> kVideoCodecRED, kVideoCodecULPFEC, kVideoCodecGeneric, and
> kVideoCodecUnknown. In RTCVideoDecoderFactory::**CreateVideoDecoder, we
> don't have video_codec_config.size. So we cannot call
> VideoDecoder::Initialize there.
>
> We can let VideoDecoder::Initialize decide. A mapping table will be
> needed. Since only VP8 is the only option that can be mapped to
> media::VideoCodecProfile. We don't need to add the code not used now.
> Thought?
>

My point was that if anything other than VP8 is requested from the factory,
it will return NULL and execution will never reach this test, so checking
for VP8ness seems redundant.


> https://codereview.chromium.**org/13890012/diff/1/content/**
>
renderer/media/rtc_video_**decoder_bridge.cc#newcode174<https://codereview.chromium.org/13890012/diff/1/content/renderer/media/rtc_video_decoder_bridge.cc#newcode174>
> content/renderer/media/rtc_**video_decoder_bridge.cc:174: base::AutoLock
> auto_lock(lock_);
> On 2013/04/26 07:08:01, posciak wrote:
>
>> Lock usage is mysterious here, especially since there is a return
>>
> after this and
>
>> no change to decoding_error_occured_.
>>
> decoding_error_occurred_ is read by WebRTC DecodingThread and set by
> decoder_message_loop_. I added comments in the header file and used
> volatile instead.
>

Adding "volatile" is virtually guaranteed to be a mistake (as it is here).
http://www.airs.com/blog/archives/154 is an entertaining read, if this is
news to you.


Unrelated, I happened to see a reinterpret_cast<> in one of the files in
this CL; please use static_cast<> where possible and explain where it isn't.

Cheers,
-a

wuchengli

Argh. I didn't know volatile in C++ and Java are different. I need to use ...

7 years, 7 months ago (2013-05-13 16:20:33 UTC) #15

wuchengli

Hi Pawel and Ami. The CL is ready for review. Please take another look. I've ...

7 years, 7 months ago (2013-05-14 15:36:04 UTC) #16

Ami GONE FROM CHROMIUM

Exciting! https://codereview.chromium.org/13890012/diff/70001/content/browser/renderer_host/render_process_host_impl.cc File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/13890012/diff/70001/content/browser/renderer_host/render_process_host_impl.cc#newcode1 content/browser/renderer_host/render_process_host_impl.cc:1: // Copyright (c) 2012 The Chromium Authors. All ...

7 years, 7 months ago (2013-05-14 22:57:24 UTC) #17

wuchengli

Hi Ami. I have several questions in the comments. I did not update a new ...

7 years, 7 months ago (2013-05-15 15:30:46 UTC) #18

Hi Ami. I have several questions in the comments. I did not update a new
patchset so you can ignore most comments. Please just answer the questions if
you have time. Thanks.

https://codereview.chromium.org/13890012/diff/70001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/13890012/diff/70001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2013/05/14 22:57:24, Ami Fischman wrote:
> Do you have numbers for the perf impact of this CL (and its related CLs in
> libjingle & webrtc)?
The initial test result looked like frame rate of VDA was lower. CPU usage was
similar. I'll test again to confirm.

I used talk_base::RateTracker to measure frame rate. Are there other ways to
measure perf besides RateTracker and CPU usage?

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:470:
decoder_worker_thread_.message_loop_proxy(), gpu_factories_));
On 2013/05/14 22:57:24, Ami Fischman wrote:
> Does this mean that the same gpu_factories will be used for every
> mediastream-backed <video> tag in a given renderer process?  That's broken: a
> given gpu_factories_ object can only be used with a single <video> tag (e.g.
> look at its impl use of waitableevents).
I'm not sure which waitableevents are you talking about. Can you give me the URL
of the source?

Do you mean we need to cretae a gpu_factories_ every time
CreatePeerConnectionFactory is called?

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:25: public
base::SupportsWeakPtr<RTCDemuxerStream> {
On 2013/05/14 22:57:24, Ami Fischman wrote:
> Exposing SupportsWeakPtr is a code smell.
> In this case it's also unnecessary, because the only user of the facility is
the
> owner of this class (RTCVideoDecoder) who can maintain its own weakptr for it!
Removed SupportsWeakPtr. I added new methods in RTCVideoDecoder to call
RTCDemuxerStream::UpdateSize and QueueBuffer. Let me know if you have other
suggestions.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:37: void ClearBuffer();
On 2013/05/14 22:57:24, Ami Fischman wrote:
> Pluralize?

Done.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:45: };
On 2013/05/14 22:57:24, Ami Fischman wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:65: return *(new
media::AudioDecoderConfig);
On 2013/05/14 22:57:24, Ami Fischman wrote:
> return media::AudioDecoderConfig();
> doesn't work?
Right. The compile error is "error: returning reference to temporary".

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:78: NOTREACHED();
On 2013/05/14 22:57:24, Ami Fischman wrote:
> Why NOTREACHED here but LOG(FATAL) at l.64?
Agree. I changed NOTREACHED to LOG(FATAL). I just found NOTREACHED is DCHECK,
not CHECK.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:128: void
RTCVideoDecoder::InitWeakPtr() {
On 2013/05/14 22:57:24, Ami Fischman wrote:
> s/Ptr/Ptrs/

Done.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:137: 
On 2013/05/14 22:57:24, Ami Fischman wrote:
> You should 
> DCHECK(decoder_message_loop_->BelongsToCurrentThread());
> in the dtor since you're relying on the weak ptrs being invalidated there.
Done. Great suggestion.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:149: DCHECK(codecSettings->codecType
== webrtc::kVideoCodecVP8);
On 2013/05/14 22:57:24, Ami Fischman wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:216: // precision. Here we use
TimeDelta as a holder to preverse the exact same
On 2013/05/14 22:57:24, Ami Fischman wrote:
> s/preverse/preserve/

Done.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:272: // We don't use statistics for
now.
On 2013/05/14 22:57:24, Ami Fischman wrote:
> TODO to do this.

Done.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:280: pipeline_status_ = status;
On 2013/05/14 22:57:24, Ami Fischman wrote:
> This is accessed from more than one thread so it should be under lock_.
This is accessed from two threads. But the sequencing is controlled by
WaitableEvent. Does the new value of pipeline_status guarantee to be visible to
the other thread?

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:281: lock_.Acquire();
On 2013/05/14 22:57:24, Ami Fischman wrote:
> FWIW I think you can replace l.281-289 with:
> 
> bool do_read = false;
> { 
>   AutoLock ...;
>   if (OK) {
>     state_ = kInitialized;
>     do_read = true;
>   }
> }
> if (do_read)
>   video_decoder->Read(...);
> 
> which runs less risk of forgetting to Release() the lock.
Done. Good suggestion.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:322: LOG(ERROR) << "FrameReady
error. status=" << status;
On 2013/05/14 22:57:24, Ami Fischman wrote:
> FWIW this is not necessarily an _error_ per se, just an unsupported path.
You are right. Changed to VLOG(1).

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:349: }
On 2013/05/14 22:57:24, Ami Fischman wrote:
> Ditto could reformat this.

Done.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder.h (right):

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.h:31: // complete callback by
|decoder_message_loop_|.
On 2013/05/14 22:57:24, Ami Fischman wrote:
> s/by/on/

Done.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.h:77: scoped_ptr<media::VideoDecoder>
video_decoder_;
On 2013/05/14 22:57:24, Ami Fischman wrote:
> scoped_ptr<> here implies ownership, but passing by raw-pointer in the ctor
> means non-ownership.    I think you should change the ctor and use .Pass()
when
> calling the ctor.

Done.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.h:103: // DecodingThread and set by
decoder_message_loop_ (guarded by |lock_|.
On 2013/05/14 22:57:24, Ami Fischman wrote:
> missing close paren

Done.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.h:104: bool decoding_error_occurred_;
On 2013/05/14 22:57:24, Ami Fischman wrote:
> Why not use state_ to indicate this?
Done. Great suggestion.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.h:106:
base::WeakPtrFactory<RTCVideoDecoder> weak_factory_;
On 2013/05/14 22:57:24, Ami Fischman wrote:
> Doco that these live & die on the decoder_message_loop_.

Done.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder_unittest.cc (right):

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_unittest.cc:36: bridge_.reset(new
RTCVideoDecoder(decoder_, loop_proxy_));
On 2013/05/14 22:57:24, Ami Fischman wrote:
> |bridge_| is now a strange name for the variable considering none of the
classes
> involved has a "Bridge" in the name.
Changed |bridge_| to |rtc_decoder_| and changed |decoder_| to |mock_decoder_|.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_unittest.cc:67:
num_delivered_frames_++;
On 2013/05/14 22:57:24, Ami Fischman wrote:
> nit: prefer pre-{de,in}crement to post-, where the side-effect is unnecessary.
> 
> (here and elsewhere)
All done.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_unittest.cc:187: EXPECT_EQ(1,
num_delivered_frames_); //XXX: lock?
On 2013/05/14 22:57:24, Ami Fischman wrote:
> If all you need is sequencing (not mutual exclusion) then using the
> WaitableEvents is enough.
Yes. It's only sequencing. The comments were removed.

Pawel Osciak

Wu-cheng, did you forget to upload PS 11? https://chromiumcodereview.appspot.com/13890012/diff/70001/content/browser/renderer_host/render_process_host_impl.cc File content/browser/renderer_host/render_process_host_impl.cc (right): https://chromiumcodereview.appspot.com/13890012/diff/70001/content/browser/renderer_host/render_process_host_impl.cc#newcode1 content/browser/renderer_host/render_process_host_impl.cc:1: // ...

7 years, 7 months ago (2013-05-15 17:26:32 UTC) #19

Ami GONE FROM CHROMIUM

> https://codereview.chromium.**org/13890012/diff/70001/** > content/browser/renderer_host/**render_process_host_impl.cc<https://codereview.chromium.org/13890012/diff/70001/content/browser/renderer_host/render_process_host_impl.cc> > File content/browser/renderer_host/**render_process_host_impl.cc (right): > > https://codereview.chromium.**org/13890012/diff/70001/** > content/browser/renderer_host/**render_process_host_impl.cc#**newcode1<https://codereview.chromium.org/13890012/diff/70001/content/browser/renderer_host/render_process_host_impl.cc#newcode1> > content/browser/renderer_host/**render_process_host_impl.cc:1: ...

7 years, 7 months ago (2013-05-15 19:42:04 UTC) #20

> https://codereview.chromium.**org/13890012/diff/70001/**
>
content/browser/renderer_host/**render_process_host_impl.cc<https://codereview.chromium.org/13890012/diff/70001/content/browser/renderer_host/render_process_host_impl.cc>
> File content/browser/renderer_host/**render_process_host_impl.cc (right):
>
> https://codereview.chromium.**org/13890012/diff/70001/**
>
content/browser/renderer_host/**render_process_host_impl.cc#**newcode1<https://codereview.chromium.org/13890012/diff/70001/content/browser/renderer_host/render_process_host_impl.cc#newcode1>
> content/browser/renderer_host/**render_process_host_impl.cc:1: //
> Copyright (c) 2012 The Chromium Authors. All rights reserved.
> On 2013/05/14 22:57:24, Ami Fischman wrote:
>
>> Do you have numbers for the perf impact of this CL (and its related
>>
> CLs in
>
>> libjingle & webrtc)?
>>
> The initial test result looked like frame rate of VDA was lower. CPU usage
> was similar.


That's disappointing, right?  Any ideas why?


> I'll test again to confirm.  I used talk_base::RateTracker to measure
> frame rate. Are there other ways to measure perf besides RateTracker and
> CPU usage?


I'd recommend testing with HD (&hd=true on apprtc, for example, sending
from a desktop that can capture/send at 720p, and use
chrome://webrtc-internals to confirm that you're really getting 720p).  For
example, on my daisy this makes for a pretty terrible received frame-rate
(<1fps) as the SW decoder falls behind.  If the HW decoder could keep up
that'd be a pretty compelling result.

https://codereview.chromium.**org/13890012/diff/70001/**
>
content/renderer/media/media_**stream_dependency_factory.cc<https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/media_stream_dependency_factory.cc>
> File content/renderer/media/media_**stream_dependency_factory.cc (right):
>
> https://codereview.chromium.**org/13890012/diff/70001/**
>
content/renderer/media/media_**stream_dependency_factory.cc#**newcode470<https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/media_stream_dependency_factory.cc#newcode470>
> content/renderer/media/media_**stream_dependency_factory.cc:**470:
> decoder_worker_thread_.**message_loop_proxy(), gpu_factories_));
> On 2013/05/14 22:57:24, Ami Fischman wrote:
>
>> Does this mean that the same gpu_factories will be used for every
>> mediastream-backed <video> tag in a given renderer process?  That's
>>
> broken: a
>
>> given gpu_factories_ object can only be used with a single <video> tag
>>
> (e.g.
>
>> look at its impl use of waitableevents).
>>
> I'm not sure which waitableevents are you talking about. Can you give me
> the URL of the source?
>

I was referring to the use of
RendererGpuVideoDecoderFactories::render_thread_async_waiter_, for example.
E.g. if two GVD's were sharing the same RGVDF instance, and each called its
CreateSharedMemory() method, then one of them could end up receiving the
Signal meant for the other at the WaitMany at
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
You need to create one RGVDF per <video> tag that uses it.

Do you mean we need to cretae a gpu_factories_ every
> time CreatePeerConnectionFactory is called?


No, because a single PeerConnectionFactory can create multiple
PeerConnections.
Further, a single PeerConnection can manage multiple streams.
This would probably all be clearer if RGVDF::CreateVideoDecodeAccelerator()
DCHECK'd if it were called more than once in a single RGVDF object.

https://codereview.chromium.**org/13890012/diff/70001/**
>
content/renderer/media/rtc_**video_decoder.cc#newcode65<https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rtc_video_decoder.cc#newcode65>
> content/renderer/media/rtc_**video_decoder.cc:65: return *(new
> media::AudioDecoderConfig);
> On 2013/05/14 22:57:24, Ami Fischman wrote:
>
>> return media::AudioDecoderConfig();
>> doesn't work?
>>
> Right. The compile error is "error: returning reference to temporary".


Urgl.  The current code is a leak.  Any reason to not just LOG(FATAL) and
avoid the need to write anything else?

https://codereview.chromium.**org/13890012/diff/70001/**
>
content/renderer/media/rtc_**video_decoder.cc#newcode280<https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rtc_video_decoder.cc#newcode280>
> content/renderer/media/rtc_**video_decoder.cc:280: pipeline_status_ =
> status;
> On 2013/05/14 22:57:24, Ami Fischman wrote:
>
>> This is accessed from more than one thread so it should be under
>>
> lock_.
> This is accessed from two threads. But the sequencing is controlled by
> WaitableEvent.


I don't follow; what WaitableEvent applies between the read in InitDecode()
and the write here?

Does the new value of pipeline_status guarantee to be visible to the other
> thread?


The general rule is that if you need to ask this question then you should
be using a lock :p

wuchengli

I spent most time rewriting the test today. I replied several comments that may need ...

7 years, 7 months ago (2013-05-16 16:05:45 UTC) #21

I spent most time rewriting the test today. I replied several comments that may
need discussion. I'll address the other review comments tomorrow. 

@Pawel: I didn't upload patchset 11 because I didn't want to confuse Ami with
lots of patchsets before addressing all the comments. I've updated my latest
patchset for your reference.

https://chromiumcodereview.appspot.com/13890012/diff/70001/content/renderer/m...
File content/renderer/media/rtc_video_decoder.cc (right):

https://chromiumcodereview.appspot.com/13890012/diff/70001/content/renderer/m...
content/renderer/media/rtc_video_decoder.cc:65: return *(new
media::AudioDecoderConfig);
On 2013/05/15 17:26:32, posciak wrote:
> On 2013/05/15 15:30:46, wuchengli wrote:
> > On 2013/05/14 22:57:24, Ami Fischman wrote:
> > > return media::AudioDecoderConfig();
> > > doesn't work?
> > Right. The compile error is "error: returning reference to temporary".
> > 
> 
> Isn't this a leak? Isn't this class supposed to maintain ownership of created
> config? Perhaps a dummy empty config in this class?
LOG(FATAL) will cause the program to terminate. This line will not be executed.
The original patchset uses a dummy config. I can change it back.

@Ami: The current patchset uses LOG(FATAL) and it doesn't remove the need of
returning an object.

https://chromiumcodereview.appspot.com/13890012/diff/70001/content/renderer/m...
content/renderer/media/rtc_video_decoder.cc:82:
DCHECK(message_loop_->BelongsToCurrentThread());
On 2013/05/15 17:26:32, posciak wrote:
> Did size update work for you with VDAs if posted from Decode()?
UpdateSize is posted from Decode() now. You mean if I changed to use
VideoDecodeAccelerator or?

https://chromiumcodereview.appspot.com/13890012/diff/70001/content/renderer/m...
content/renderer/media/rtc_video_decoder.cc:155:
frame_size_.SetSize(codecSettings->width, codecSettings->height);
On 2013/05/15 17:26:32, posciak wrote:
> On 2013/05/14 22:57:24, Ami Fischman wrote:
> > Didn't the internal thread result in saying these are the container's
> > dimensions, which may not be the coded size at all?  I think you need the
> > latter.  If you were using GVDAH directly you'd get to see what size the VDA
> > found in the bitstream, which would be the definitive place to know the
coded
> > size.
> 
> How does frame_size_ relate to visible size? Do we assume it's mb-aligned or
> don't care?
@Ami: UpdateSize is needed here because GpuVideoDecoder::Initialize has a DCHECK
against (0,0) size VideoDecoderConfig. We need to set the size or move
GpuVideoDecoder::Initialize to Decode(). Using GVDAH is another story. I'll
study how to use it.

@Pawel: See the following discussion from an internal thread. Frame size in
Decode should be the same as visible size. Does GpuVideoDecoder have any
requirement about visible size? I don't see mb-aligned is required.

- VideoDecoder::InitDecode has parameter codecSettings. VideoDecoder::Decode has
parameter inputFrame. What's the relationship between
codecSettings->width/height and inputFrame._encodedWidth/_encodedHeight?

The width and height provided in codecSettings can be ignored. They are there
for codecs which doesn't have width/height in the bitstream.

- To use GpuVideoDecoder, I need to create a VideoDecoderConfig, whose
constructor needs coded_size, visible_rect, and natural_size. Are
EncodedImage._encodedWidth/_encodedHeight always multiples of 16? Is it correct
to use coded_size=visible_rect=natural_size=(_enocdedWidth,_encodedHeight)? I
took a quick look at VP8 data format. Maybe _encodedWidth and _encodedHeight are
actual image dimension and not necessarily evenly divisible by 16?

encodedWidth/Height are parsed from the vp8 bitstream and should represent the
actual image dimensions. Seems to make sense that those can be set to
(encodedWidth, encodedHeight), although it's not completely clear to me what
they represent.

https://chromiumcodereview.appspot.com/13890012/diff/70001/content/renderer/m...
content/renderer/media/rtc_video_decoder.cc:162:
base::Unretained(video_decoder_.get()),
On 2013/05/14 22:57:24, Ami Fischman wrote:
> Why is this (and the next) Unretained safe?
Unretained is safe because the WaitableEvent next line will wait until it
finishes.

https://chromiumcodereview.appspot.com/13890012/diff/70001/content/renderer/m...
File content/renderer/media/rtc_video_decoder_factory.cc (right):

https://chromiumcodereview.appspot.com/13890012/diff/70001/content/renderer/m...
content/renderer/media/rtc_video_decoder_factory.cc:20: if (type ==
webrtc::kVideoCodecVP8) {
On 2013/05/15 17:26:32, posciak wrote:
> Is there a chance this could be moved to actual decoder to make this decision
at
> some point? Perhaps a TODO at least?
From my understanding of the code, I think CreateVideoDecoder should return NULL
if the codec is not supported. It seems WebRTC will register the codec as the
supported one. Maybe I can ask webrtc eng about this.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

I was thinking to move GpuVideoDecoder::Initialize here but we don't have video
frame size here. Initialize has a DCHECK against (0,0) size in
VideoDecoderConfig.

All VideoCodecType values are kVideoCodecVP8, kVideoCodecI420, kVideoCodecRED,
kVideoCodecULPFEC, kVideoCodecGeneric, and kVideoCodecUnknown. It is very
different from media::VideoCodecProfile. The only value can be mapped to
VideoCodecProfile is kVideoCodecVP8.

Ami GONE FROM CHROMIUM

> > https://chromiumcodereview.**appspot.com/13890012/diff/** > 70001/content/renderer/media/**rtc_video_decoder.cc#**newcode155<https://chromiumcodereview.appspot.com/13890012/diff/70001/content/renderer/media/rtc_video_decoder.cc#newcode155> > content/renderer/media/rtc_**video_decoder.cc:155: > frame_size_.SetSize(**codecSettings->width, codecSettings->height); > On 2013/05/15 17:26:32, posciak ...

7 years, 7 months ago (2013-05-17 02:30:37 UTC) #22

wuchengli

Hi Ami. I've updated a patchset that uses VDA in RTCVideoDecoder. The CL still needs ...

7 years, 7 months ago (2013-05-19 14:39:33 UTC) #23

wuchengli

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/media_stream_dependency_factory.cc File content/renderer/media/media_stream_dependency_factory.cc (right): https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/media_stream_dependency_factory.cc#newcode470 content/renderer/media/media_stream_dependency_factory.cc:470: decoder_worker_thread_.message_loop_proxy(), gpu_factories_)); On 2013/05/15 15:30:46, wuchengli wrote: > On ...

7 years, 7 months ago (2013-05-20 14:32:15 UTC) #24

Ami GONE FROM CHROMIUM

> - The role of RTCVideoDecoder is similar to GpuVideoDecoder now. I copied > some ...

7 years, 7 months ago (2013-05-20 18:17:53 UTC) #25

> - The role of RTCVideoDecoder is similar to GpuVideoDecoder now. I copied
> some
> code from GpuVideoDecoder. The difference is RTCVideoDecoder only has
> vda_loop_proxy_. It has no gvd_loop_proxy_ or VDAClientProxy. Is this OK?


"gvd_loop_proxy_" is the media pipeline's thread, and only exists so that
(SW) video decode doesn't serialize with JS execution and everything else
the main thread is doing.  This is fine for the HW decode case, where only
IPCs happen in the renderer process (low load) and the IPCs go through the
main thread anyway.
VDAClientProxy is needed to deal with the fact that GVD wants to be
callable on the media thread while VDA must be called on the main thread,
and VDA::Client callbacks get made on the main thread but must twiddle GVD
bits that are at home on the media thread.  Since in your case there is no
"media thread" you should be able to drop both of these.
If the purpose of VDAClientProxy is still murky I recommend you take a
stroll through https://codereview.chromium.org/12989009/.

Note that your CL hasn't completely dropped these things; e.g.
Initialize()/SetVDA() still look like Initialize() is called on the media
thread (hence needing to post SetVDA() to vda_loop_proxy_).  In fact you
can inline SetVDA() into Initialize() (since Initialize() is called on the
main thread a.k.a. vda_loop_proxy_) and inline Initialize() into the ctor
since it never returns false (and RVD is no longer a media::VideoDecoder).

I got a strange problem. When I used vda_loop_proxy_ (compositor thread) to
> call
> RendererGpuVideoDecoderFactori**es::CreateSharedMemory, it got stuck in
> Wait and
> AsyncCreateSharedMemory is not called at all.


Are you using a Debug build?  It sounds to me like you're not and getting
getting a hang because CSM() posts to the child thread and then waits for
that posted task to finish, *even though* it's already on the child thread.
 If you were using a Debug build I'd expect you to trigger the DCHECK_NE at
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

You should be able to make CSM() callable on any thread by doing what the
ctor and ReadPixels do: if not already on the thread you'd PostTask to then
do the current code; otherwise (already on the thread that'd get the
PostTask) then skip the PostTask+Wait and simply call the "Async" version
of the method.

I ended up using WebRTC decoding thread to call CreateSharedMemory.


Yeah, you shouldn't need to do that :)


> I thought ChildThread::current()->**message_loop() is the main thread of
> renderer process.
>

It is.

I was thinking to move the creation of gpu_factories to
> RTCVideoDecoderFactory::**CreateVideoDecoder. CreateVideoDecoder is called
> on WebRTC decoding thread. So it will need to post the task back to main
> renderer thread to create the gpu_factories. Do you think that's the
> right place?
>

It might have been, if the decoder factory ran on the main thread.  I don't
think it is the right place though b/c it runs on a different thread and
the main thread is already blocked when CreateVideoDecoder is called (as
you found out, below).  I expect that the right place to create RGVDF's is
in RenderViewImpl, sometime before the call chain kicks off that triggers
the eventual CreateVideoDecoder.  See below.


> I tried to implement this today. But the main renderer thread does not
> run the task at all. I'm still trying to figure out why.
>

Most likely this is caused by the main thread being blocked on the webrtc
decoding thread already, so posting back to the main thread wouldn't work.


> What method or data structure can map to one mediastream-backed <video>
> tag? Does one WebMediaPlayerMS mean one mediastream-backed <video> tag?
> https://code.google.com/p/**chromium/codesearch#chromium/**
> src/content/renderer/render_**view_impl.cc&q=**
>
WebMediaPlayerMS&sq=package:**chromium&type=cs&l=2700<https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/render_view_impl.cc&q=WebMediaPlayerMS&sq=package:chromium&type=cs&l=2700>


Yes, I believe so.  Passing WMPMS's ctor a just-created RGVDF is probably
the right thing to do.

wuchengli

> Note that your CL hasn't completely dropped these things; e.g. > Initialize()/SetVDA() still look ...

7 years, 7 months ago (2013-05-23 16:25:46 UTC) #26

> Note that your CL hasn't completely dropped these things; e.g.
> Initialize()/SetVDA() still look like Initialize() is called on the media
> thread (hence needing to post SetVDA() to vda_loop_proxy_).  In fact you
> can inline SetVDA() into Initialize() (since Initialize() is called on the
> main thread a.k.a. vda_loop_proxy_) and inline Initialize() into the ctor
> since it never returns false (and RVD is no longer a media::VideoDecoder).
Initialize is called on Chrome_libJingle_WorkerThread, so trampoline is needed.
It can return false when the codec is not supported. Sorry I didn't implement it
in the last patchset.
> 
> I got a strange problem. When I used vda_loop_proxy_ (compositor thread) to
> > call
> > RendererGpuVideoDecoderFactori**es::CreateSharedMemory, it got stuck in
> > Wait and
> > AsyncCreateSharedMemory is not called at all.
> Are you using a Debug build?  It sounds to me like you're not and getting
> getting a hang because CSM() posts to the child thread and then waits for
> that posted task to finish, *even though* it's already on the child thread.
My GDB started working and I know why. vda_loop_proxy is compositor thread. Main
thread periodically commits and waits for compositor thread to finish. When the
compositor thread calls CreateSharedMemory and waits for main thread, deadlock
will happen. So I still use webrtc decoding thread to call CreateSharedMemory.

https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr...
> > What method or data structure can map to one mediastream-backed <video>
> > tag? Does one WebMediaPlayerMS mean one mediastream-backed <video> tag?
> > Yes, I believe so.  Passing WMPMS's ctor a just-created RGVDF is probably
> the right thing to do.
Can I always create a new MediaStreamImpl in
RenderViewImpl::EnsureMediaStreamImpl()? Then a new MediaStreamDependencyFactory
and RendererGpuVideoDecoderFactories will be created every time.

wuchengli

Ami and Pawel. The CL is ready for review. After using VDA, the code is ...

7 years, 7 months ago (2013-05-23 16:50:46 UTC) #27

Ami and Pawel. The CL is ready for review. After using VDA, the code is indeed
cleaner and the performance should be better. Please ignore the unittest because
it's not updated yet.

How do you usually measure fps? I put a RateTracker in libjingle. That is the
number of frames libjingle received from the decoder. I wonder whether we should
measure the number of frames rendered per second. Also, is the fps counter
(--show-fps-counter) interesting to us?

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:189: if (state_ == kUninitialized ||
decode_complete_callback_ == NULL) {
On 2013/05/15 17:26:32, posciak wrote:
> As Ami suggested, it'd probably be enough to combine this and previous if into
> one if state_ == kDecoding (kInitialized is not a valid state for Decode()
call
> either, right?).
I changed decoding_error_occurred_ to state_==kDecodeError. The number of states
are reduced and kInitialized is a valid state now.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:195: LOG(ERROR) << "Missing frames
or not completed frames.";
On 2013/05/15 17:26:32, posciak wrote:
> This is not a fatal error, iiuc we might be running into this quite often
during
> a conference, perhaps this is too much spam for LOG(ERROR)?
Good point. Changed to DLOG.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.cc:248: decoder_waiter_.Wait();
On 2013/05/15 17:26:32, posciak wrote:
> Would DCHECK(state_==kUninitialized) after this be useful?
Do you think state_ may not be updated? renderer_gpu_video_decoder_factories.cc
has several patterns like this. Actually I'm not very sure about the visibility
of the variables using WaitableEvent.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder.h (right):

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.h:27: // GpuVideoDecoder.
webrtc::VideoDecoder methods likes InitDecode() or Decode()
On 2013/05/15 17:26:32, posciak wrote:
> s/likes/like

Done.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.h:79: // The message loop to run
callbacks. This should be the same as the main
On 2013/05/15 17:26:32, posciak wrote:
> s/callbacks/callbacks on/

Done.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder.h:86: // Used to wait GpuVideoDecoder
calls to complete. InitDecode(), Release(),
On 2013/05/15 17:26:32, posciak wrote:
> s/wait/wait for/

Done.

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
File content/renderer/media/rtc_video_decoder_factory.cc (right):

https://codereview.chromium.org/13890012/diff/70001/content/renderer/media/rt...
content/renderer/media/rtc_video_decoder_factory.cc:20: if (type ==
webrtc::kVideoCodecVP8) {
On 2013/05/16 16:05:45, wuchengli wrote:
> On 2013/05/15 17:26:32, posciak wrote:
> > Is there a chance this could be moved to actual decoder to make this
decision
> at
> > some point? Perhaps a TODO at least?
> From my understanding of the code, I think CreateVideoDecoder should return
NULL
> if the codec is not supported. It seems WebRTC will register the codec as the
> supported one. Maybe I can ask webrtc eng about this.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
> 
> I was thinking to move GpuVideoDecoder::Initialize here but we don't have
video
> frame size here. Initialize has a DCHECK against (0,0) size in
> VideoDecoderConfig.
> 
> All VideoCodecType values are kVideoCodecVP8, kVideoCodecI420, kVideoCodecRED,
> kVideoCodecULPFEC, kVideoCodecGeneric, and kVideoCodecUnknown. It is very
> different from media::VideoCodecProfile. The only value can be mapped to
> VideoCodecProfile is kVideoCodecVP8.
The decision is moved to actual decoder now.

Ami GONE FROM CHROMIUM

On Thu, May 23, 2013 at 9:25 AM, <wuchengli@chromium.org> wrote: > Note that your CL ...

7 years, 7 months ago (2013-05-23 16:56:19 UTC) #28

On Thu, May 23, 2013 at 9:25 AM, <wuchengli@chromium.org> wrote:

> Note that your CL hasn't completely dropped these things; e.g.
>> Initialize()/SetVDA() still look like Initialize() is called on the media
>> thread (hence needing to post SetVDA() to vda_loop_proxy_).  In fact you
>> can inline SetVDA() into Initialize() (since Initialize() is called on the
>> main thread a.k.a. vda_loop_proxy_) and inline Initialize() into the ctor
>> since it never returns false (and RVD is no longer a media::VideoDecoder).
>>
> Initialize is called on Chrome_libJingle_WorkerThread, so trampoline is
> needed.
> It can return false when the codec is not supported. Sorry I didn't
> implement it
> in the last patchset.
>

Where is the ctor called?  If it's on the child thread then
Initialize/SetVDA can be inlined into the ctor.
(and Initialize can turn into a IsInitialized or somesuch to let the worker
know about failures).

 I got a strange problem. When I used vda_loop_proxy_ (compositor thread) to
>> > call
>> > RendererGpuVideoDecoderFactori****es::CreateSharedMemory, it got stuck
>> in
>> > Wait and
>> > AsyncCreateSharedMemory is not called at all.
>> Are you using a Debug build?  It sounds to me like you're not and getting
>> getting a hang because CSM() posts to the child thread and then waits for
>> that posted task to finish, *even though* it's already on the child
>> thread.
>>
> My GDB started working and I know why. vda_loop_proxy is compositor
> thread. Main
> thread periodically commits and waits for compositor thread to finish.


Can you point out the exact sequence of waits & locks taken involved in
this last sentence?  I'd like to understand it better.

https://code.google.com/p/**chromium/codesearch#chromium/**
> src/cc/trees/thread_proxy.cc&**q=thread_proxy.cc&sq=package:**
>
chromium&type=cs&l=758<https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_proxy.cc&q=thread_proxy.cc&sq=package:chromium&type=cs&l=758>
>
>> > What method or data structure can map to one mediastream-backed <video>
>> > tag? Does one WebMediaPlayerMS mean one mediastream-backed <video> tag?
>> > Yes, I believe so.  Passing WMPMS's ctor a just-created RGVDF is
>> probably
>> the right thing to do.
>>
> Can I always create a new MediaStreamImpl in
> RenderViewImpl::**EnsureMediaStreamImpl()?


I'm not sure that's legit because of RenderViewImpl::userMediaClient().
 wjia/grunell probably know for sure; I suggest you shoot them an email
asking.

Ami GONE FROM CHROMIUM

On Thu, May 23, 2013 at 9:50 AM, <wuchengli@chromium.org> wrote: > How do you usually ...

7 years, 7 months ago (2013-05-23 17:00:59 UTC) #29

wuchengli

On Fri, May 24, 2013 at 12:56 AM, Ami Fischman <fischman@chromium.org>wrote: > On Thu, May ...

7 years, 7 months ago (2013-05-23 17:17:52 UTC) #30

On Fri, May 24, 2013 at 12:56 AM, Ami Fischman <fischman@chromium.org>wrote:

> On Thu, May 23, 2013 at 9:25 AM, <wuchengli@chromium.org> wrote:
>
>> Note that your CL hasn't completely dropped these things; e.g.
>>> Initialize()/SetVDA() still look like Initialize() is called on the media
>>> thread (hence needing to post SetVDA() to vda_loop_proxy_).  In fact you
>>> can inline SetVDA() into Initialize() (since Initialize() is called on
>>> the
>>> main thread a.k.a. vda_loop_proxy_) and inline Initialize() into the ctor
>>> since it never returns false (and RVD is no longer a
>>> media::VideoDecoder).
>>>
>> Initialize is called on Chrome_libJingle_WorkerThread, so trampoline is
>> needed.
>> It can return false when the codec is not supported. Sorry I didn't
>> implement it
>> in the last patchset.
>>
> Where is the ctor called?  If it's on the child thread then
> Initialize/SetVDA can be inlined into the ctor.
> (and Initialize can turn into a IsInitialized or somesuch to let the
> worker know about failures).
>
- In the latest patchset, RTCVideoDecoder ctor and Initialize are both in
RTCVideoDecoderFactory::CreateVideoDecoder(). So they are called
on Chrome_libJingle_WorkerThread.
- RTCVideoDecoderFactory ctor is called on the child thread. But, we don't
have the codec type in RTCVideoDecoderFactory ctor. We need to let
VDA::Initialize to decide whether the codec is supported.

>
>  I got a strange problem. When I used vda_loop_proxy_ (compositor thread)
>>> to
>>> > call
>>> > RendererGpuVideoDecoderFactori****es::CreateSharedMemory, it got
>>> stuck in
>>> > Wait and
>>> > AsyncCreateSharedMemory is not called at all.
>>> Are you using a Debug build?  It sounds to me like you're not and getting
>>> getting a hang because CSM() posts to the child thread and then waits for
>>> that posted task to finish, *even though* it's already on the child
>>> thread.
>>>
>> My GDB started working and I know why. vda_loop_proxy is compositor
>> thread. Main
>> thread periodically commits and waits for compositor thread to finish.
>
>
> Can you point out the exact sequence of waits & locks taken involved in
> this last sentence?  I'd like to understand it better.
>
I don't have it with me now. I'll send it to you tomorrow. It was an
interesting callstack.

>
> https://code.google.com/p/**chromium/codesearch#chromium/**
>> src/cc/trees/thread_proxy.cc&**q=thread_proxy.cc&sq=package:**
>>
chromium&type=cs&l=758<https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_proxy.cc&q=thread_proxy.cc&sq=package:chromium&type=cs&l=758>
>>
>>> > What method or data structure can map to one mediastream-backed <video>
>>> > tag? Does one WebMediaPlayerMS mean one mediastream-backed <video> tag?
>>> > Yes, I believe so.  Passing WMPMS's ctor a just-created RGVDF is
>>> probably
>>> the right thing to do.
>>>
>> Can I always create a new MediaStreamImpl in
>> RenderViewImpl::**EnsureMediaStreamImpl()?
>
>
> I'm not sure that's legit because of RenderViewImpl::userMediaClient().
>  wjia/grunell probably know for sure; I suggest you shoot them an email
> asking.
>
Will do.

Ami GONE FROM CHROMIUM

On Thu, May 23, 2013 at 10:17 AM, Wu-Cheng Li (李務誠) <wuchengli@chromium.org>wrote: > I got ...

7 years, 7 months ago (2013-05-27 01:07:16 UTC) #31

wuchengli

GpuVideoDecoder makes all the calls to VDA on the message loop of gpu_factories (the compositor ...

7 years, 7 months ago (2013-05-27 05:38:44 UTC) #32

GpuVideoDecoder makes all the calls to VDA on the message loop of gpu_factories
(the compositor thread). The role of RTCVideoDecoder is the same as
GpuVideoDecoder. So to be consistent, it should also use the same thread to make
all the calls to VDA. I did wonder why GpuVideoDecoder uses the compositor
thread to run VDA because I was not sure VDA was related to compositing. If VDA
is not required to run on compositor thread, I can try changing RTCVideoDecoder
to use the main thread or a new thread.

GpuVideoDecoder uses the message loop of gpu_factories for VDA:
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/gpu_...
The loop of gpu_factories is compositor thread:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

On 2013/05/27 01:07:16, Ami Fischman wrote:
> On Thu, May 23, 2013 at 10:17 AM, Wu-Cheng Li (李務誠)
> <wuchengli@chromium.org>wrote:
> 
> >  I got a strange problem. When I used vda_loop_proxy_ (compositor thread)
> >>>> to
> >>>> > call
> >>>> > RendererGpuVideoDecoderFactori****es::CreateSharedMemory, it got
> >>>> stuck in
> >>>> > Wait and
> >>>> > AsyncCreateSharedMemory is not called at all.
> >>>> Are you using a Debug build?  It sounds to me like you're not and
> >>>> getting
> >>>> getting a hang because CSM() posts to the child thread and then waits
> >>>> for
> >>>> that posted task to finish, *even though* it's already on the child
> >>>> thread.
> >>>>
> >>> My GDB started working and I know why. vda_loop_proxy is compositor
> >>> thread. Main
> >>> thread periodically commits and waits for compositor thread to finish.
> >>
> >>
> >> Can you point out the exact sequence of waits & locks taken involved in
> >> this last sentence?  I'd like to understand it better.
> >>
> > I don't have it with me now. I'll send it to you tomorrow. It was an
> > interesting callstack.
> >
> 
> In another email Wu-Cheng pointed out that the main thread waits for the
> compositor thread in ThreadProxy::BeginFrameOnMainThread().
> 
> @wuchengli: I've context-switched a bit here; can you remind me why you
> were using the compositor thread instead of the main thread to call
> CreateSharedMemory (and generally run the RTCVideoDecoder on)?
> (it may well be because you tried to do it on the main thread and that
> didn't work, because CSM needs to be enhanced a bit to work that way (the
> way the ctor does))

Ami GONE FROM CHROMIUM

On Sun, May 26, 2013 at 10:38 PM, <wuchengli@chromium.org> wrote: > I can try changing ...

7 years, 6 months ago (2013-05-28 05:46:50 UTC) #33

wuchengli

The test is updated. I added mock_gpu_video_decoder_factories.h and mock_video_decode_accelerator.h. - I'll try using a different ...

7 years, 6 months ago (2013-05-28 15:00:59 UTC) #34

Ami GONE FROM CHROMIUM

See below for replies to comments that seemed like questions or things that are still ...

7 years, 6 months ago (2013-05-29 21:11:45 UTC) #35

wuchengli

The CL is ready for review. Please take another look. RTCVideoDecoder and RendererGpuVideoDecoderFactories (RGVDF) cannot ...

7 years, 6 months ago (2013-06-10 12:33:42 UTC) #36

wuchengli

https://codereview.chromium.org/13890012/diff/130001/content/renderer/media/rtc_video_decoder.cc File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/13890012/diff/130001/content/renderer/media/rtc_video_decoder.cc#newcode339 content/renderer/media/rtc_video_decoder.cc:339: aborted_waiter_.Signal(); This makes sure the functions won't block forever ...

7 years, 6 months ago (2013-06-10 15:24:22 UTC) #37

Ami GONE FROM CHROMIUM

https://codereview.chromium.org/13890012/diff/130001/content/browser/renderer_host/render_process_host_impl.cc File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/13890012/diff/130001/content/browser/renderer_host/render_process_host_impl.cc#newcode1 content/browser/renderer_host/render_process_host_impl.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights ...

7 years, 6 months ago (2013-06-11 23:48:05 UTC) #38

wuchengli

Hi John. Please take a look at exynos_video_decode_accelerator.h and see if you have any concern. ...

7 years, 6 months ago (2013-06-12 09:17:47 UTC) #39

Pawel Osciak

mostly nits https://codereview.chromium.org/13890012/diff/130001/content/common/gpu/media/exynos_video_decode_accelerator.h File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://codereview.chromium.org/13890012/diff/130001/content/common/gpu/media/exynos_video_decode_accelerator.h#newcode93 content/common/gpu/media/exynos_video_decode_accelerator.h:93: kDpbOutputBufferExtraCount = 8, This adds almost 42MB ...

7 years, 6 months ago (2013-06-12 23:38:22 UTC) #40

sheu

https://codereview.chromium.org/13890012/diff/130001/content/common/gpu/media/exynos_video_decode_accelerator.h File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://codereview.chromium.org/13890012/diff/130001/content/common/gpu/media/exynos_video_decode_accelerator.h#newcode93 content/common/gpu/media/exynos_video_decode_accelerator.h:93: kDpbOutputBufferExtraCount = 8, On 2013/06/12 23:38:22, posciak wrote: > ...

7 years, 6 months ago (2013-06-13 01:24:25 UTC) #41

wuchengli

I address all review comments except the one in DismissPictureBuffer. I still need to check ...

7 years, 6 months ago (2013-06-13 10:28:06 UTC) #42

Ami GONE FROM CHROMIUM

So close! https://chromiumcodereview.appspot.com/13890012/diff/130001/content/browser/renderer_host/render_process_host_impl.cc File content/browser/renderer_host/render_process_host_impl.cc (right): https://chromiumcodereview.appspot.com/13890012/diff/130001/content/browser/renderer_host/render_process_host_impl.cc#newcode1 content/browser/renderer_host/render_process_host_impl.cc:1: // Copyright (c) 2012 The Chromium Authors. ...

7 years, 6 months ago (2013-06-13 20:10:02 UTC) #43

sheu

The magic free-up-two-buffers deal is here: https://chromiumcodereview.appspot.com/16866016/

7 years, 6 months ago (2013-06-13 20:44:56 UTC) #44

Ami GONE FROM CHROMIUM

I had to dig a bit to re-figure-out why RGVDF was so specific about its ...

7 years, 6 months ago (2013-06-13 21:22:58 UTC) #45

Ami GONE FROM CHROMIUM

On Thu, Jun 13, 2013 at 2:22 PM, Ami Fischman <fischman@chromium.org> wrote: > ISTM the ...

7 years, 6 months ago (2013-06-13 22:02:05 UTC) #46

sheu

https://chromiumcodereview.appspot.com/13890012/diff/147001/content/common/gpu/media/exynos_video_decode_accelerator.h File content/common/gpu/media/exynos_video_decode_accelerator.h (right): https://chromiumcodereview.appspot.com/13890012/diff/147001/content/common/gpu/media/exynos_video_decode_accelerator.h#newcode88 content/common/gpu/media/exynos_video_decode_accelerator.h:88: kMfcInputBufferCount = 12, Each one of these are 512 ...

7 years, 6 months ago (2013-06-14 04:15:36 UTC) #47

wuchengli

All done. Please take another look. Sorry I forgot to put rebase in a separate ...

7 years, 6 months ago (2013-06-18 16:11:22 UTC) #48

All done. Please take another look. Sorry I forgot to put rebase in a separate
CL.

https://codereview.chromium.org/13890012/diff/147001/content/common/gpu/media...
File content/common/gpu/media/exynos_video_decode_accelerator.h (right):

https://codereview.chromium.org/13890012/diff/147001/content/common/gpu/media...
content/common/gpu/media/exynos_video_decode_accelerator.h:93:
kDpbOutputBufferExtraCount = 8,
On 2013/06/14 04:15:37, sheu wrote:
> The revert of the tearing hack landed.  Try the peformance again without
> increasing this buffer count.
After rebasing chrome and chrome OS source, the performance became better. I
don't know why. I reverted all the changes in this file.

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/n...
File content/renderer/media/native_handle_impl.h (right):

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/n...
content/renderer/media/native_handle_impl.h:17: virtual void SetHandle(void*
handle);
On 2013/06/13 20:10:03, Ami Fischman wrote:
> style: virtual functions must have OVERRIDE declarations.
> In general you should probably build with the chromium-style-checker plugin
> enabled so that you can fix these before the trybots fail on you; details on
how
> to do that are at https://code.google.com/p/chromium/wiki/Clang#Using_plugins
> TL;DR is add clang_use_chrome_plugins=1 to $GYP_DEFINES and re-run gyp.
> More info on the things checked at
> http://dev.chromium.org/developers/coding-style/chromium-style-checker-errors
OVERRIDE is added. I had problems building with clang. I'll try again.

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/r...
File content/renderer/media/renderer_gpu_video_decoder_factories.cc (right):

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/r...
content/renderer/media/renderer_gpu_video_decoder_factories.cc:82:
DCHECK(!message_loop_->BelongsToCurrentThread());
On 2013/06/13 20:10:03, Ami Fischman wrote:
> Why remove the sync path?
This patchset uses a new thread as |message_loop_|, so sync path was not
required. Now we are changing it back to the compositing thread. The sync path
is added again.

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/r...
File content/renderer/media/renderer_gpu_video_decoder_factories.h (right):

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/r...
content/renderer/media/renderer_gpu_video_decoder_factories.h:124:
DISALLOW_IMPLICIT_CONSTRUCTORS(RendererGpuVideoDecoderFactories);
On 2013/06/13 20:10:03, Ami Fischman wrote:
> Now that you have a no-arg ctor this is sort of a lie.
> You want DISALLOW_COPY_AND_ASSIGN and to declare a private do-nothing ctor
> explicitly at the top of the private: section.

Done.

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:19: // A shared memory segment and
its allocated size.
On 2013/06/13 20:10:03, Ami Fischman wrote:
> this implies ownership but the dtor neither unmaps nor deletes the
SharedMemory
> pointer.  Explicate that this is unowned?

Done.

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:23: base::SharedMemory* shm;
On 2013/06/13 20:10:03, Ami Fischman wrote:
> possibly can const'ify all the members here and in BufferData.
> (without that public mutable fields become something readers need to worry
> about)

Done.

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:32: // The data of a bitstream
buffer.
On 2013/06/13 20:10:03, Ami Fischman wrote:
> s/The data/Metadata/

Done.

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:268:
picture_buffers_in_decoder_.erase(it);
On 2013/06/13 20:10:03, Ami Fischman wrote:
> Propagating unaddressed comment to avoid losing it:
> What if the texture is being drawn from?
> I think you need an analog of r204513 (and r205347) for this to actually be
> right.
> The good news is I suspect nothing calls VDA::Client::DPB for your codepath.  
> Maybe replace impl with NOTREACHED and TODO to adapt CLs above to here?
I merged r204513 and r205347 to my CL. It's still easy to merge the code now.
WebRTC eng also mentioned VP8 resolution can change on the fly. This may be
useful in the future.

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:308:
decoded_image.CreateEmptyFrame(width, height, width, height / 2, width / 2);
On 2013/06/13 20:10:03, Ami Fischman wrote:
> Add TODO to remove the malloc.

Done.

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:513: it != input_buffer_data_.end();
++it) {
On 2013/06/13 20:10:03, Ami Fischman wrote:
> On 2013/06/13 10:28:07, wuchengli wrote:
> > On 2013/06/11 23:48:05, Ami Fischman wrote:
> > > This is unlikely to impact perf, and I think this is just copy/pasta from
> GVD,
> > > but:
> > > it's probably true that the buffer we're looking for here is at the back
of
> > the
> > > list; might be better to use rbegin/rend.  Just a thought.
> > I found the element was not removed from the list. I added the code to
remove
> it
> > when it's found.
> 
> This is a bug; see below.
> 
> > Then the buffer should be in the front. Right?
> 
> When I made my comment above I forgot that we don't remove metadata and
instead
> drop when the list exceeds a certain size.  So actually starting at the
> beginning _is_ better, because most runs will find their quarry 6-16 buffers
in
> from the start and the full list length is 128.
> 
> > What does the comment in RecordBufferData mean? Does it only apply to H264?
> 
> Yes.

Done.

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:519: input_buffer_data_.erase(it);
On 2013/06/13 20:10:03, Ami Fischman wrote:
> BUG: Drop this - multiple pictures can be delivered for a single
> BitstreamBuffer.

Done.

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:525:
RTCVideoDecoder::BufferData::~BufferData() {}
On 2013/06/13 20:10:03, Ami Fischman wrote:
> Move to top of file to be next to BufferData's other methods.

Done.

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder_factory.h (right):

https://codereview.chromium.org/13890012/diff/147001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder_factory.h:28: // completing.
On 2013/06/13 20:10:03, Ami Fischman wrote:
> This is clearer, but even clearer might be "Child thread is blocked while this
> runs."

Done.

Ami GONE FROM CHROMIUM

https://codereview.chromium.org/13890012/diff/189001/content/browser/renderer_host/render_process_host_impl.cc File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/13890012/diff/189001/content/browser/renderer_host/render_process_host_impl.cc#newcode1 content/browser/renderer_host/render_process_host_impl.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights ...

7 years, 6 months ago (2013-06-18 21:54:29 UTC) #49

wuchengli

All done. Please take another look. https://codereview.chromium.org/13890012/diff/189001/content/browser/renderer_host/render_process_host_impl.cc File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/13890012/diff/189001/content/browser/renderer_host/render_process_host_impl.cc#newcode1 content/browser/renderer_host/render_process_host_impl.cc:1: // Copyright (c) ...

7 years, 6 months ago (2013-06-19 13:01:12 UTC) #50

All done. Please take another look.

https://codereview.chromium.org/13890012/diff/189001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/13890012/diff/189001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2013/06/18 21:54:29, Ami Fischman wrote:
> Can you add perf #s to the CL description?

Done.

https://codereview.chromium.org/13890012/diff/189001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/13890012/diff/189001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:81: // renderer thread shuts down.
On 2013/06/18 21:54:29, Ami Fischman wrote:
> This means that you don't actually need the waiter to be a class member (which
> is never touched after the ctor returns).
> Instead you can have a stack-local WaitableEvent and wait for it here, passing
> its address to the bound Initialize call.

Done.

https://codereview.chromium.org/13890012/diff/189001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:223: while (buffers_delayed_.size()
> 0) {
On 2013/06/18 21:54:29, Ami Fischman wrote:
> l.223-226 is:
> buffers_to_be_decoded_.insert(buffers_to_be_decoded_.end(),
>     buffers_delayed_.begin(), buffers_delayed_.end());
> (which is a no-op of b_d_ is empty)
Done. I had to remove const in BufferData because STL uses assignment operator.
Otherwise the code generated compile error: "non-static const member 'const
size_t content::RTCVideoDecoder::BufferData::size', can't use default assignment
operator"

https://codereview.chromium.org/13890012/diff/189001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:254: if (!vda_)
On 2013/06/18 21:54:29, Ami Fischman wrote:
> why not do this at the top of the method?
Done.

GpuVideoDecoder puts it in the same place. I agree RTCVideoDecoder shouldn't
need to create the textures if |vda_| is gone.

https://codereview.chromium.org/13890012/diff/189001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:291: .insert(std::make_pair(id,
buffer_to_dismiss)).second;
On 2013/06/18 21:54:29, Ami Fischman wrote:
> period goes on previous line 
> (git cl format is your friend)
I did run git cl format. I completely follow its recommendation now. This style
was generated by git cl format.

https://codereview.chromium.org/13890012/diff/189001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:403: swap(buffers_to_be_decoded_,
buffers_delayed_);
On 2013/06/18 21:54:29, Ami Fischman wrote:
> std::swap() to avoid debugging pain down the line.

Done.

https://codereview.chromium.org/13890012/diff/189001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:477: vda_->Reset();
On 2013/06/18 21:54:29, Ami Fischman wrote:
> if (!vda_)
>   return;
> first
Done. I also added the check in RequestBufferDecode.

https://codereview.chromium.org/13890012/diff/189001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:535:
std::deque<std::pair<SHMBuffer*, BufferData> >& queue) {
On 2013/06/18 21:54:29, Ami Fischman wrote:
> pass by pointer
> (never non-const ref in chromium style)

Done.

https://codereview.chromium.org/13890012/diff/189001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:536: base::AutoLock
auto_lock(lock_);
On 2013/06/18 21:54:29, Ami Fischman wrote:
> It is a code-smell to take a member lock in a method that doesn't otherwise
> touch any class state.  Here the class state comes through the argument, which
> the caller is responsible for, so the usual prescription would be to move the
> lock to the caller.  However in this case the only caller is the dtor, and
> locking in a dtor is pointless, so I think you can just drop this autolock.
You are right. Done.

https://codereview.chromium.org/13890012/diff/189001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder_factory.cc (right):

https://codereview.chromium.org/13890012/diff/189001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder_factory.cc:19: // thread is blocked
when CreateVideoDecoder runs.
On 2013/06/18 21:54:29, Ami Fischman wrote:
> TODO that this supports only one VDA-powered <video> tag at a time, and that
> IWBN to remove this restriction.

Done.

https://codereview.chromium.org/13890012/diff/189001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder_factory.cc:22:
decoder_.reset(decoder.release());
On 2013/06/18 21:54:29, Ami Fischman wrote:
> FWIW instead of 
> new+Init+.release/.reset
> you could have a static factory method
> RTCVideoDecoder::Create(const scoped_refptr<m::GVD::F>& factories)
> which returned NULL on Init failure.  Your choice.
Sounds good. Done.

https://codereview.chromium.org/13890012/diff/189001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder_factory.cc:48: decoder_.reset(decoder);
On 2013/06/18 21:54:29, Ami Fischman wrote:
> Probably want to do something like VDA::Reset() and then wait for
> NotifyResetDone before reusing this decoder.
VDA::Release() should have been called, which does the same thing as Reset().
Decode() will queue the decode buffers until NotifyResetDone.

Ami GONE FROM CHROMIUM

https://codereview.chromium.org/13890012/diff/215001/content/renderer/media/media_stream_dependency_factory.cc File content/renderer/media/media_stream_dependency_factory.cc (right): https://codereview.chromium.org/13890012/diff/215001/content/renderer/media/media_stream_dependency_factory.cc#newcode499 content/renderer/media/media_stream_dependency_factory.cc:499: if (gpu_factories.get() != NULL) Is the .get() really required? ...

7 years, 6 months ago (2013-06-19 18:28:58 UTC) #51

wuchengli

All done. Please take another look. If it looks good, I want to separate native_handle_impl.cc/h ...

7 years, 6 months ago (2013-06-20 07:27:04 UTC) #52

wuchengli

The deadlock is fixed. Now RTCVideoDecoder calls RGVDF::CreateSharedMemory asynchronously. Please take another look.

7 years, 6 months ago (2013-06-24 14:48:13 UTC) #53

Ami GONE FROM CHROMIUM

https://codereview.chromium.org/13890012/diff/232001/content/browser/renderer_host/render_process_host_impl.cc File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/13890012/diff/232001/content/browser/renderer_host/render_process_host_impl.cc#newcode1 content/browser/renderer_host/render_process_host_impl.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights ...

7 years, 6 months ago (2013-06-26 00:11:58 UTC) #54

wuchengli

All done. Please take another look. https://codereview.chromium.org/13890012/diff/232001/content/browser/renderer_host/render_process_host_impl.cc File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/13890012/diff/232001/content/browser/renderer_host/render_process_host_impl.cc#newcode1 content/browser/renderer_host/render_process_host_impl.cc:1: // Copyright (c) ...

7 years, 5 months ago (2013-06-28 15:08:44 UTC) #55

All done. Please take another look.

https://codereview.chromium.org/13890012/diff/232001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/13890012/diff/232001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2013/06/26 00:11:58, Ami Fischman wrote:
> Please don't address comments and rebase in the same patchset - it makes
> reviewing patchset diffs unnecessarily noisy.
> Instead, address comments, upload patchset, then rebase, and upload next
> patchset.
> Thanks.

Done.

https://codereview.chromium.org/13890012/diff/232001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2013/06/26 00:11:58, Ami Fischman wrote:
> Looks like you lost the race with
> https://chromiumcodereview.appspot.com/14199002
> Hopefully rebasing on that won't be _too_ painful.
Rebased. It didn't take long. BTW, what are mailbox and sync point?

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:40: size_t sisze);
On 2013/06/26 00:11:58, Ami Fischman wrote:
> typo: sisze

Done.

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:202: SendPendingBuffersForDecode();
On 2013/06/26 00:11:58, Ami Fischman wrote:
> This is a strange place to place this call.  What's the thinking here?
The buffers should be sent to VDA in order. If any pending buffer is not sent,
the current buffer in |inputImage| should be appended to the pending queue.

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:336: // WebRTC expects no frame
callback after Release. Drop the frame if VDA is
On 2013/06/26 00:11:58, Ami Fischman wrote:
> Given the frame callback is run on a thread other than the one on which the
> Release is called, and no lock is held across both invocations, how can this
be
> guaranteed?
> 
> IOW, state_ may not be RESETTING here but then be set to RESETTING between the
> release of the lock at l.341 and the call to Decoded() at l.360.
> 
> I suspect you need to hold the state lock here while calling the callback.
You are right. I need to hold the lock while calling the callback. The callback
calls into WebRTC. We cannot return from Release while the callback is running.

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:478: if
(buffer_data->bitstream_buffer_id < reset_bitstream_buffer_id_) {
On 2013/06/26 00:11:58, Ami Fischman wrote:
> This doesn't handle the wraparound you have at 30bits.
> You want something akin to
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
Done. Good catch...

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:528: &webrtc_buffers_.front();
On 2013/06/26 00:11:58, Ami Fischman wrote:
> why * instead of const&?
No special reason. Is there a preference to use const& for local variables in a
function?

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:549: // Clone the input image and
save it to the queue.
On 2013/06/26 00:11:58, Ami Fischman wrote:
> Is it unsafe to simply swap _buffer pointers?
This should be OK because WebRTC will allocate a new buffer. Unfortunately
inputImage is passed as const reference in Decode().

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:641: // Create three shared memory
at once so we don't need to trampoline to the
On 2013/06/26 00:11:58, Ami Fischman wrote:
> Why 3 and not kMaxInFlightDecodes?
> (and method name & doco need to reflect this)
Because the message can be posted twice or more before CreateSHM is actually
called. It's easy to have kMaxInFlightDecodes * 2 buffers and half of them is
wasted. 

I've changed it to always keeping an extra SHM.

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.h (right):

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.h:106: void SendBufferForDecode(const
webrtc::EncodedImage& inputImage,
On 2013/06/26 00:11:58, Ami Fischman wrote:
> s/inputImage/input_image/ since this isn't an override for a webrtc method.

Done.

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.h:107: SHMBuffer* shm_buffer,
On 2013/06/26 00:11:58, Ami Fischman wrote:
> indent is off here and elsewhere
Indent looks correct. I've run git cl format. Where are you referring to?

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.h:107: SHMBuffer* shm_buffer,
They are all output params. It's a pointer because I need to save it to stl
deque. It's not const because make_pair requires it to be non-const.

I didn't use AppendBufferToDecodeQueue and DrainDecodeQueue because
SaveToPendingBuffers needs an extra memcpy. We own the buffers of
|webrtc_buffers_|. But we don't own |inputImage._buffer| and we cannot swap the
buffer. So I don't want to put them in the same queue.

On 2013/06/26 00:11:58, Ami Fischman wrote:
> style: output params follow input params
> But this isn't really an output param; you're passing shm_buffer so it can be
> used by SBFD but you don't care about the return value.  Maybe I'm missing
> something but instead of
> SendBufferFD
> SendPendingBuffersFD
> SaveToPendingBuffers
> why not have:
> AppendBufferToDecodeQueue
> DrainDecodeQueue
> where the former unconditionally adds the input_image to a queue, and the
latter
> drains as many images as it has SHMs for?  I think that will simplify both the
> calling code and the impl.
> In particular because running out of SHMs because of in-flight decodes is one
> thing, but having SHMs that are too small is another.  (the former should just
> wait till a SHM can be recycled, the latter should create new SHMs)

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.h:135: // not own returned pointer.
On 2013/06/26 00:11:58, Ami Fischman wrote:
> Doco meaning of NULL return

Done.

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.h:197: // DecodingThread.
On 2013/06/26 00:11:58, Ami Fischman wrote:
> Generally vars not protected by locks go above the lock in class layout.

Done.

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:357: webrtc::TextureVideoFrame
decoded_image(width, height, timestamp, 0, handle);
malloc has been removed. I'll update webrtc CL soon.

wuchengli

https://codereview.chromium.org/13890012/diff/267001/content/renderer/media/rtc_video_decoder.cc File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/13890012/diff/267001/content/renderer/media/rtc_video_decoder.cc#newcode39 content/renderer/media/rtc_video_decoder.cc:39: static const size_t kMaxNumOfPendingBuffers = 300; kMaxNumSharedMemorySegments and kMaxNumOfPendingBuffers ...

7 years, 5 months ago (2013-06-28 15:45:40 UTC) #56

Ami GONE FROM CHROMIUM

https://codereview.chromium.org/13890012/diff/232001/content/browser/renderer_host/render_process_host_impl.cc File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/13890012/diff/232001/content/browser/renderer_host/render_process_host_impl.cc#newcode1 content/browser/renderer_host/render_process_host_impl.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights ...

7 years, 5 months ago (2013-06-28 17:03:59 UTC) #57

https://codereview.chromium.org/13890012/diff/232001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/13890012/diff/232001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2013/06/28 15:08:44, wuchengli wrote:
> On 2013/06/26 00:11:58, Ami Fischman wrote:
> > Looks like you lost the race with
> > <crxref style="background-image:
url(chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/codereview.png);">https://chromiumcodereview.appspot.com/14199002</crxref>
> > Hopefully rebasing on that won't be _too_ painful.
> Rebased. It didn't take long. BTW, what are mailbox and sync point?

gpu/GLES2/extensions/CHROMIUM/CHROMIUM_texture_mailbox.txt
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt

I'll fwd you a thread in which I asked the same question :)

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/13890012/diff/232001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:528: &webrtc_buffers_.front();
On 2013/06/28 15:08:44, wuchengli wrote:
> On 2013/06/26 00:11:58, Ami Fischman wrote:
> > why * instead of const&?
> No special reason. Is there a preference to use const& for local variables in
a
> function?

Mainly I think what I look for is less ambiguity about mutability.  So it's the
const I look for over the &-vs-*, but once you have a const* that can never be
NULL the usual practice is to use const&.
(because in chromium code the only const*'s you see are things that could have
been const& except they also need to handle NULL :))

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:508: bool
RTCVideoDecoder::IsBufferAfterReset(int32 id_buffer, int32 id_reset) {
If Reset/Release have never been called, id_reset will be 0 in each call to this
method.  For id_buffer values in [ID_HALF, ID_LAST] this method will return
false!

Special-case to early-return true in the case of id_reset==0 (in which case you
probably want to make the sentinel value -1, not 0)?

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:512: return diff < ID_HALF;
worth a test for the interesting edge conditions?

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:566: // Queued too many buffers.
Something goes wrong.
This is not a super-satisfying block of code...
IWBN to know what sort of things you're worried about.

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:572: memcpy(buffer,
input_image._buffer, input_image._length);
Add a TODO to extend the Decode() interface to take a non-const ptr to the frame
and add a method to the frame that'll swap buffers with another?

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:684:
available_shm_segments_.push_back(shm_buffer);
does it make sense to SendPendingBuffersForDecode here?

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.h (right):

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.h:119: int SaveToPendingBuffers(const
webrtc::EncodedImage& input_image,
returning int where a bool will do is a webrtc'ism that is unstylish in chromium
(and google3).  Please return true/false to indicate success/failure, resp.

https://codereview.chromium.org/13890012/diff/267001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/13890012/diff/267001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:39: static const size_t
kMaxNumOfPendingBuffers = 300;
On 2013/06/28 15:45:41, wuchengli wrote:
> kMaxNumSharedMemorySegments and kMaxNumOfPendingBuffers are necessary.
Otherwise
> chrome will run out of memory when things go wrong.

What sort of things can go wrong?

wuchengli

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/rtc_video_decoder.cc File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/rtc_video_decoder.cc#newcode508 content/renderer/media/rtc_video_decoder.cc:508: bool RTCVideoDecoder::IsBufferAfterReset(int32 id_buffer, int32 id_reset) { On 2013/06/28 17:04:00, ...

7 years, 5 months ago (2013-06-29 05:01:29 UTC) #58

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:508: bool
RTCVideoDecoder::IsBufferAfterReset(int32 id_buffer, int32 id_reset) {
On 2013/06/28 17:04:00, Ami Fischman wrote:
> If Reset/Release have never been called, id_reset will be 0 in each call to
this
> method.  For id_buffer values in [ID_HALF, ID_LAST] this method will return
> false!
Right. Suppose we add sentinel value -1. When id_reset is set, the maximum
difference is still ID_HALF. Adding sentinel value will only increase the
maximum difference in the first run. This is the limitation of this method, the
same as timeutils.cc. If WebRTC runs 30fps, ID_HALF is 207 days. It should be
enough. The current code uses 30 bits. I can change it to 31 bits and ID_HALF
will become 414 days.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
> 
> Special-case to early-return true in the case of id_reset==0 (in which case
you
> probably want to make the sentinel value -1, not 0)?

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:566: // Queued too many buffers.
Something goes wrong.
On 2013/06/28 17:04:00, Ami Fischman wrote:
> This is not a super-satisfying block of code...
> IWBN to know what sort of things you're worried about.
I saw apprtc loopback showed the self image running and did not connect. After
10 seconds or so, RTCVideoDecoder queued too many buffers and chrome ran out of
memory. When this happened, it happened when start. I don't know where the
problem was. It only occurred around once per several days.

The other possibility is if VDA has a bug and it does not return any frame, this
can happen.

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:684:
available_shm_segments_.push_back(shm_buffer);
On 2013/06/28 17:04:00, Ami Fischman wrote:
> does it make sense to SendPendingBuffersForDecode here?
PutSHM can be called by the compositor thread or the child thread. The threading
is simpler when |webrtc_buffers_| is only accessed by WebRTC DecodingThread.
SendPendingBuffersForDecode is only be called in the very beginning. When SHM is
enough, it won't run any more. Also, a new SHM will be created before the queue
size reaches 0. SendPendingBuffersForDecode should be called rarely. Having the
latency of one frame before sending pending buffers should be OK.

I thought of creating kMaxInFlightDecodes SHM at once in InitDecode to make it
more efficient.

Ami GONE FROM CHROMIUM

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/rtc_video_decoder.cc File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/rtc_video_decoder.cc#newcode508 content/renderer/media/rtc_video_decoder.cc:508: bool RTCVideoDecoder::IsBufferAfterReset(int32 id_buffer, int32 id_reset) { On 2013/06/29 05:01:30, ...

7 years, 5 months ago (2013-06-29 05:23:02 UTC) #59

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:508: bool
RTCVideoDecoder::IsBufferAfterReset(int32 id_buffer, int32 id_reset) {
On 2013/06/29 05:01:30, wuchengli wrote:
> On 2013/06/28 17:04:00, Ami Fischman wrote:
> > If Reset/Release have never been called, id_reset will be 0 in each call to
> this
> > method.  For id_buffer values in [ID_HALF, ID_LAST] this method will return
> > false!
> Right. Suppose we add sentinel value -1. When id_reset is set, the maximum
> difference is still ID_HALF. Adding sentinel value will only increase the
> maximum difference in the first run. This is the limitation of this method,
the
> same as timeutils.cc. If WebRTC runs 30fps, ID_HALF is 207 days. It should be
> enough. The current code uses 30 bits. I can change it to 31 bits and ID_HALF
> will become 414 days.

My point was that the question of whether |id_buffer| precedes |id_reset| is
moot when Reset/Release were never called.  So you don't have to pay this
(admittedly extremely small) cost of limiting to ID_HALF (or even to ID_LAST+1)
buffers in that case.

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:566: // Queued too many buffers.
Something goes wrong.
On 2013/06/29 05:01:30, wuchengli wrote:
> On 2013/06/28 17:04:00, Ami Fischman wrote:
> > This is not a super-satisfying block of code...
> > IWBN to know what sort of things you're worried about.
> I saw apprtc loopback showed the self image running and did not connect. After
> 10 seconds or so, RTCVideoDecoder queued too many buffers and chrome ran out
of
> memory. When this happened, it happened when start. I don't know where the
> problem was. It only occurred around once per several days.

I'd rather have the bug manifest so it can be tracked down rather than paper
over it.

> The other possibility is if VDA has a bug and it does not return any frame,
this
> can happen.

Would it be better to drop decode requests that were too old, do you think?
(and recycle their SHMs)

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:684:
available_shm_segments_.push_back(shm_buffer);
On 2013/06/29 05:01:30, wuchengli wrote:
> On 2013/06/28 17:04:00, Ami Fischman wrote:
> > does it make sense to SendPendingBuffersForDecode here?
> PutSHM can be called by the compositor thread or the child thread. The
threading
> is simpler when |webrtc_buffers_| is only accessed by WebRTC DecodingThread.
> SendPendingBuffersForDecode is only be called in the very beginning. When SHM
is
> enough, it won't run any more. Also, a new SHM will be created before the
queue
> size reaches 0. SendPendingBuffersForDecode should be called rarely. Having
the
> latency of one frame before sending pending buffers should be OK.

My point is that if there are pending decodes that are just waiting for a
vehicle to get to the GPU process it would be good to kick them off ASAP to keep
the pipeline full.
Maybe the RequestBufferDecode at the bottom of NotifyEndOfBitstreamBuffer
already does this?

> I thought of creating kMaxInFlightDecodes SHM at once in InitDecode to make it
> more efficient.

This seems like a good idea.

wuchengli

All done. Please take another look. A bad news is the decode latency becomes worse ...

7 years, 5 months ago (2013-07-02 10:34:25 UTC) #60

All done. Please take another look. A bad news is the decode latency becomes
worse after rebasing for mailbox. Decode latency is around 350ms now (was
100ms). I think the symptom is still the busy GPU main thread. I won't address
it in this CL.

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:508: bool
RTCVideoDecoder::IsBufferAfterReset(int32 id_buffer, int32 id_reset) {
On 2013/06/29 05:23:03, Ami Fischman wrote:
> On 2013/06/29 05:01:30, wuchengli wrote:
> > On 2013/06/28 17:04:00, Ami Fischman wrote:
> > > If Reset/Release have never been called, id_reset will be 0 in each call
to
> > this
> > > method.  For id_buffer values in [ID_HALF, ID_LAST] this method will
return
> > > false!
> > Right. Suppose we add sentinel value -1. When id_reset is set, the maximum
> > difference is still ID_HALF. Adding sentinel value will only increase the
> > maximum difference in the first run. This is the limitation of this method,
> the
> > same as timeutils.cc. If WebRTC runs 30fps, ID_HALF is 207 days. It should
be
> > enough. The current code uses 30 bits. I can change it to 31 bits and
ID_HALF
> > will become 414 days.
> 
> My point was that the question of whether |id_buffer| precedes |id_reset| is
> moot when Reset/Release were never called.  So you don't have to pay this
> (admittedly extremely small) cost of limiting to ID_HALF (or even to
ID_LAST+1)
> buffers in that case.
Done. Added ID_INVALID=-1 as sentinel value.

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:512: return diff < ID_HALF;
On 2013/06/28 17:04:00, Ami Fischman wrote:
> worth a test for the interesting edge conditions?

Done.

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:566: // Queued too many buffers.
Something goes wrong.
On 2013/06/29 05:23:03, Ami Fischman wrote:
> On 2013/06/29 05:01:30, wuchengli wrote:
> > On 2013/06/28 17:04:00, Ami Fischman wrote:
> > > This is not a super-satisfying block of code...
> > > IWBN to know what sort of things you're worried about.
> > I saw apprtc loopback showed the self image running and did not connect.
After
> > 10 seconds or so, RTCVideoDecoder queued too many buffers and chrome ran out
> of
> > memory. When this happened, it happened when start. I don't know where the
> > problem was. It only occurred around once per several days.
> 
> I'd rather have the bug manifest so it can be tracked down rather than paper
> over it.
I added error log here. I'm not sure the issue still exists. I'll keep an eye on
it.
> 
> > The other possibility is if VDA has a bug and it does not return any frame,
> this
> > can happen.
> 
> Would it be better to drop decode requests that were too old, do you think?
> (and recycle their SHMs)
When RTCVideoDecoder::Decode returns error, WebRTC will trigger a
RTCVideoDecoder::Reset, which will reset VDA. It should be better.

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:572: memcpy(buffer,
input_image._buffer, input_image._length);
On 2013/06/28 17:04:00, Ami Fischman wrote:
> Add a TODO to extend the Decode() interface to take a non-const ptr to the
frame
> and add a method to the frame that'll swap buffers with another?

Done.

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:684:
available_shm_segments_.push_back(shm_buffer);
I've changed the code. Now there are two buffer queue - |decode_buffers_| and
|pending_buffers_|. If SHM is available, DecodingThread puts the buffer to
|decode_buffers_|. If not, |pending_buffers_|. RequstBufferDecode moves
|pending_buffers_| to |decode_buffers_| if SHM is available. Then
RequstBufferDecode sends |decode_buffers_| to VDA.

RequestBufferDecode at the bottom of NotifyEndOfBitstreamBuffer and CreateSHM
both do this now.

I've changed InitDecode to request to create |kMaxInFlightDecodes| buffers.
On 2013/06/29 05:23:03, Ami Fischman wrote:
> On 2013/06/29 05:01:30, wuchengli wrote:
> > On 2013/06/28 17:04:00, Ami Fischman wrote:
> > > does it make sense to SendPendingBuffersForDecode here?
> > PutSHM can be called by the compositor thread or the child thread. The
> threading
> > is simpler when |webrtc_buffers_| is only accessed by WebRTC DecodingThread.
> > SendPendingBuffersForDecode is only be called in the very beginning. When
SHM
> is
> > enough, it won't run any more. Also, a new SHM will be created before the
> queue
> > size reaches 0. SendPendingBuffersForDecode should be called rarely. Having
> the
> > latency of one frame before sending pending buffers should be OK.
> 
> My point is that if there are pending decodes that are just waiting for a
> vehicle to get to the GPU process it would be good to kick them off ASAP to
keep
> the pipeline full.
> Maybe the RequestBufferDecode at the bottom of NotifyEndOfBitstreamBuffer
> already does this?
> 
> > I thought of creating kMaxInFlightDecodes SHM at once in InitDecode to make
it
> > more efficient.
> 
> This seems like a good idea.

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.h (right):

https://codereview.chromium.org/13890012/diff/262001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.h:119: int SaveToPendingBuffers(const
webrtc::EncodedImage& input_image,
On 2013/06/28 17:04:00, Ami Fischman wrote:
> returning int where a bool will do is a webrtc'ism that is unstylish in
chromium
> (and google3).  Please return true/false to indicate success/failure, resp.

Done.

https://codereview.chromium.org/13890012/diff/267001/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/13890012/diff/267001/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:677:
DCHECK(main_loop_proxy_->BelongsToCurrentThread());
I found this function is problematic because CreateSHM can be run by the child 
thread when RTCVideoDecoder is being destroyed or after RVD is destoryed. So I
created another thread and make sure the thread stops in destructor. Let me know
how you think.

Ami GONE FROM CHROMIUM

:( to perf loss. LGTM % minor suggestions for cleanup https://codereview.chromium.org/13890012/diff/283002/content/renderer/media/rtc_video_decoder.cc File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/13890012/diff/283002/content/renderer/media/rtc_video_decoder.cc#newcode22 ...

7 years, 5 months ago (2013-07-02 23:36:56 UTC) #61

wuchengli

All done. Please take another look. https://codereview.chromium.org/13890012/diff/283002/content/renderer/media/rtc_video_decoder.cc File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/13890012/diff/283002/content/renderer/media/rtc_video_decoder.cc#newcode228 content/renderer/media/rtc_video_decoder.cc:228: if (no_pending_buffers) On ...

7 years, 5 months ago (2013-07-03 09:09:37 UTC) #62

All done. Please take another look.

https://codereview.chromium.org/13890012/diff/283002/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder.cc (right):

https://codereview.chromium.org/13890012/diff/283002/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:228: if (no_pending_buffers)
On 2013/07/02 23:36:57, Ami Fischman wrote:
> setting this under lock but then inspecting it without holding the lock
> automatically makes me wonder whether the value would be different if you
> reacquired the lock here before inspecting pending_buffers_.
It should be OK because the only place that increases 
|pending_buffers_| is in the bottom of this function. auto_lock(lock_) has been
moved to the top of this function so this is not a problem anymore.

https://codereview.chromium.org/13890012/diff/283002/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:608:
SaveToDecodeBuffers(*input_image, shm_buffer.Pass(), *buffer_data);
Done. My concern was SaveToPendingBuffers and SaveToDecodeBuffers had memcpy. I
wasn't sure holding a lock during 1-8KB memcpy was too long. From the
experiment, the decode latency didn't seem to change.
On 2013/07/02 23:36:57, Ami Fischman wrote:
> The locking in this function is wonky.  Would things be a lot clearer if
> SaveToDecodeBuffers was renamed SaveToDecodeBuffers_Locked, and both
> MovePendingBuffersToDecodeBuffers() and RTCVideoDecoder::Decode() just had an
> AutoLock at the top of their bodies?
> 
> (anytime you acquire & release a lock multiple times in a function, questions
> arise as to what invariants you're really ok with changing from under you, and
> what invariants' changes will be hard to find & fix bugs)

https://codereview.chromium.org/13890012/diff/283002/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:710: size_t size_to_allocate =
std::max(min_size, kSharedMemorySegmentBytes);
On 2013/07/02 23:36:57, Ami Fischman wrote:
> can go outside loop

Done.

https://codereview.chromium.org/13890012/diff/283002/content/renderer/media/r...
content/renderer/media/rtc_video_decoder.cc:717:
PutSHM(scoped_ptr<SHMBuffer>(new SHMBuffer(shm, size_to_allocate)));
On 2013/07/02 23:36:57, Ami Fischman wrote:
> Ditto would PutSHM_Locked() make more sense?

Done.

https://codereview.chromium.org/13890012/diff/283002/content/renderer/media/r...
File content/renderer/media/rtc_video_decoder_unittest.cc (right):

https://codereview.chromium.org/13890012/diff/283002/content/renderer/media/r...
content/renderer/media/rtc_video_decoder_unittest.cc:172:
EXPECT_TRUE(rtc_decoder_->IsBufferAfterReset(RTCVideoDecoder::ID_LAST,
On 2013/07/02 23:36:57, Ami Fischman wrote:
> For this and l.160 what does it mean that id X is always "after" reset id X?
Right. It's a little bit confusing. I changed the implementation to buffer id X
is not after reset id X.

Ami GONE FROM CHROMIUM

LGTM https://codereview.chromium.org/13890012/diff/296001/content/renderer/media/rtc_video_decoder.cc File content/renderer/media/rtc_video_decoder.cc (right): https://codereview.chromium.org/13890012/diff/296001/content/renderer/media/rtc_video_decoder.cc#newcode534 content/renderer/media/rtc_video_decoder.cc:534: return diff < ID_HALF; Do you think this ...

7 years, 5 months ago (2013-07-03 16:45:56 UTC) #63

wuchengli

As I mentioned earlier, I'm going to separate content/renderer/media/native_handle_impl.h/cc to another CL so I can ...

7 years, 5 months ago (2013-07-04 15:46:33 UTC) #64

wuchengli

Hi Ami. I removed native_handle related code and this CL is stand-alone. So I could ...

7 years, 5 months ago (2013-07-08 14:55:20 UTC) #65

piman

mostly nits. https://codereview.chromium.org/13890012/diff/345001/content/renderer/render_thread_impl.cc File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/13890012/diff/345001/content/renderer/render_thread_impl.cc#newcode875 content/renderer/render_thread_impl.cc:875: RenderThreadImpl::GetGpuFactories() { nit: can you add a ...

7 years, 5 months ago (2013-07-08 20:53:38 UTC) #67

wuchengli

All done. Please take another look. https://codereview.chromium.org/13890012/diff/345001/content/renderer/render_thread_impl.cc File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/13890012/diff/345001/content/renderer/render_thread_impl.cc#newcode875 content/renderer/render_thread_impl.cc:875: RenderThreadImpl::GetGpuFactories() { On ...

7 years, 5 months ago (2013-07-09 07:39:17 UTC) #68

piman

https://codereview.chromium.org/13890012/diff/345001/content/renderer/render_thread_impl.cc File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/13890012/diff/345001/content/renderer/render_thread_impl.cc#newcode888 content/renderer/render_thread_impl.cc:888: CAUSE_FOR_GPU_LAUNCH_VIDEODECODEACCELERATOR_INITIALIZE); On 2013/07/09 07:39:19, wuchengli wrote: > On 2013/07/08 ...

7 years, 5 months ago (2013-07-09 19:35:21 UTC) #69

wuchengli

https://codereview.chromium.org/13890012/diff/345001/content/renderer/render_thread_impl.cc File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/13890012/diff/345001/content/renderer/render_thread_impl.cc#newcode888 content/renderer/render_thread_impl.cc:888: CAUSE_FOR_GPU_LAUNCH_VIDEODECODEACCELERATOR_INITIALIZE); On 2013/07/09 19:35:22, piman wrote: > On 2013/07/09 ...

7 years, 5 months ago (2013-07-09 22:29:25 UTC) #70

piman

On Tue, Jul 9, 2013 at 3:29 PM, <wuchengli@chromium.org> wrote: > > https://codereview.chromium.**org/13890012/diff/345001/** > content/renderer/render_**thread_impl.cc<https://codereview.chromium.org/13890012/diff/345001/content/renderer/render_thread_impl.cc> ...

7 years, 5 months ago (2013-07-09 22:37:03 UTC) #71

On Tue, Jul 9, 2013 at 3:29 PM, <wuchengli@chromium.org> wrote:

>
> https://codereview.chromium.**org/13890012/diff/345001/**
>
content/renderer/render_**thread_impl.cc<https://codereview.chromium.org/13890012/diff/345001/content/renderer/render_thread_impl.cc>
> File content/renderer/render_**thread_impl.cc (right):
>
> https://codereview.chromium.**org/13890012/diff/345001/**
>
content/renderer/render_**t<https://codereview.chromium.org/13890012/diff/345001/content/renderer/render_thread_impl.cc#newcode888>
> *elSync recreates the channel if it has been*
> *lost. Do we want to keep this behavior? If not, GetGpuChannel() can be*
> *used because it checks if the channel is
lost.*hread_impl.cc#newcode888<https://codereview.chromium.org/13890012/diff/345001/content/renderer/render_thread_impl.cc#newcode888>
> content/renderer/render_**thread_impl.cc:888:
> CAUSE_FOR_GPU_LAUNCH_**VIDEODECODEACCELERATOR_**INITIALIZE);
> On 2013/07/09 19:35:22, piman wrote:
>
>> On 2013/07/09 07:39:19, wuchengli wrote:
>> > On 2013/07/08 20:53:39, piman wrote:
>> > > note: this can fail in theory (return NULL).
>> > > Actually, instead of EstablishGpuChannelSync, you can directly
>>
> take
>
>> > > gpu_channel_. It may still be NULL if the context was lost.
>> > I added NULL check. I don't see why gpu_channel_ can be directly
>>
> used.
>
>> > EstablishGpuChannelSync may have not been called.
>>
>
>  It has to have been called it you have a non-NULL context3d, since you
>>
> need the
>
>> GpuChannelHost to create it.
>>
> I see. EstablishGpuChannelSync recreates the channel if it has been
> lost. Do we want to keep this behavior? If not, GetGpuChannel() can be
> used because it checks if the channel is lost.


If the channel is lost, but the context3d isn't NULL, recreating the
channel would actually be worse because then the context and the VDA
wouldn't talk to the same channel, causing all sorts of issues. So I think
it's better to use the current channel.

>
>
> https://codereview.chromium.**org/13890012/diff/368002/**
>
content/renderer/render_**thread_impl.cc<https://codereview.chromium.org/13890012/diff/368002/content/renderer/render_thread_impl.cc>
> File content/renderer/render_**thread_impl.cc (right):
>
> https://codereview.chromium.**org/13890012/diff/368002/**
>
content/renderer/render_**thread_impl.cc#newcode892<https://codereview.chromium.org/13890012/diff/368002/content/renderer/render_thread_impl.cc#newcode892>
> content/renderer/render_**thread_impl.cc:892: gpu_channel_host,
> factories_loop, context3d);
> On 2013/07/09 19:35:22, piman wrote:
>
>> nit: if the statement spans more than 1 line, braces are required per
>>
> style
>
>> guide.
>>
>
> Done.
>
>
https://codereview.chromium.**org/13890012/<https://codereview.chromium.org/1...
>