Issue 126823007: [PPAPI] API thunk for video media stream track.

Issue 126823007: [PPAPI] API thunk for video media stream track. (Closed)

Created:
6 years, 11 months ago by Peng

Modified:
6 years, 11 months ago

Reviewers:
dmichael (off chromium), bbudge, yzshen1

CC:
chromium-reviews, yusukes+watch_chromium.org, jam, yzshen+watch_chromium.org, joi+watch-content_chromium.org, teravest+watch_chromium.org, darin-cc_chromium.org, raymes+watch_chromium.org, nfullagar1, piman+watch_chromium.org, noelallen1, binji, ihf+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@video_track_thunk

Visibility:
Public.

More Reviews

Description

[PPAPI] API thunk for video media stream track. BUG=330851 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244065

Patch Set 1 #

Patch Set 2 : Update #

Patch Set 3 : Update #

Patch Set 4 : Update #

Total comments: 16

Patch Set 5 : Update #

Total comments: 4

Patch Set 6 : Update #

Total comments: 2

Patch Set 7 : Update #

Total comments: 6

Patch Set 8 : Fix review issues #

Patch Set 9 : Rebase #

Created: 6 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+274 lines, -15 lines)			Patch
M	chrome/browser/component_updater/ppapi_utils.cc	View	1 2 3 4 5 6 7 8	2 chunks	+2 lines, -0 lines	0 comments	Download
M	content/renderer/pepper/plugin_module.cc	View	1 2 3 4 5 6 7 8	2 chunks	+2 lines, -0 lines	0 comments	Download
M	ppapi/api/ppb_media_stream_video_track.idl	View	1 2 3 4 5 6 7	3 chunks	+5 lines, -4 lines	0 comments	Download
M	ppapi/api/ppb_video_frame.idl	View	1 2 3 4 5	3 chunks	+5 lines, -0 lines	0 comments	Download
M	ppapi/c/ppb_media_stream_video_track.h	View	1 2 3 4 5 6 7	2 chunks	+2 lines, -5 lines	0 comments	Download
M	ppapi/c/ppb_video_frame.h	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	ppapi/cpp/media_stream_video_track.h	View	1 2 3 4 5 6 7	1 chunk	+1 line, -5 lines	0 comments	Download
M	ppapi/ppapi_shared.gypi	View	1 2 3 4 5 6 7 8	2 chunks	+4 lines, -0 lines	0 comments	Download
M	ppapi/ppapi_sources.gypi	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -0 lines	0 comments	Download
M	ppapi/proxy/interface_list.cc	View	1 2 3 4 5 6 7 8	2 chunks	+2 lines, -0 lines	0 comments	Download
M	ppapi/shared_impl/resource.h	View	1 2 3 4 5 6 7 8	2 chunks	+2 lines, -0 lines	0 comments	Download
	ppapi/thunk/interfaces_ppb_public_dev_channel.h	View	1 2 3 4	1 chunk	+3 lines, -0 lines	0 comments	Download
A	ppapi/thunk/ppb_media_stream_video_track_api.h	View	1 2 3 4 5 6 7	1 chunk	+27 lines, -0 lines	0 comments	Download
A	ppapi/thunk/ppb_media_stream_video_track_thunk.cc	View	1 2 3 4 5	1 chunk	+97 lines, -0 lines	0 comments	Download
A	ppapi/thunk/ppb_video_frame_api.h	View	1	1 chunk	+28 lines, -0 lines	0 comments	Download
A	ppapi/thunk/ppb_video_frame_thunk.cc	View	1 2 3 4	1 chunk	+92 lines, -0 lines	0 comments	Download

Messages

Total messages: 14 (0 generated)

Expand Messages | Collapse Messages

Peng

Hi, This is the thunk part of the media stream video track API. PTAL. Thanks. ...

6 years, 11 months ago (2014-01-08 16:53:50 UTC) #1

yzshen1

A few nits. Thanks! https://codereview.chromium.org/126823007/diff/150001/ppapi/thunk/interfaces_ppb_public_dev.h File ppapi/thunk/interfaces_ppb_public_dev.h (right): https://codereview.chromium.org/126823007/diff/150001/ppapi/thunk/interfaces_ppb_public_dev.h#newcode23 ppapi/thunk/interfaces_ppb_public_dev.h:23: PROXIED_IFACE(PPB_MEDIASTREAMVIDEOTRACK_INTERFACE_0_1, I mean, they should ...

6 years, 11 months ago (2014-01-08 17:56:11 UTC) #2

Peng

https://codereview.chromium.org/126823007/diff/150001/ppapi/thunk/interfaces_ppb_public_dev.h File ppapi/thunk/interfaces_ppb_public_dev.h (right): https://codereview.chromium.org/126823007/diff/150001/ppapi/thunk/interfaces_ppb_public_dev.h#newcode23 ppapi/thunk/interfaces_ppb_public_dev.h:23: PROXIED_IFACE(PPB_MEDIASTREAMVIDEOTRACK_INTERFACE_0_1, On 2014/01/08 17:56:11, yzshen1 wrote: > I mean, ...

6 years, 11 months ago (2014-01-08 18:51:25 UTC) #3

yzshen1

https://codereview.chromium.org/126823007/diff/150001/ppapi/c/ppb_media_stream_video_track.h File ppapi/c/ppb_media_stream_video_track.h (right): https://codereview.chromium.org/126823007/diff/150001/ppapi/c/ppb_media_stream_video_track.h#newcode22 ppapi/c/ppb_media_stream_video_track.h:22: * Defines the <code>PPB_MediaStreamVideoTrack</code> interface. Used for Why this ...

6 years, 11 months ago (2014-01-08 19:01:44 UTC) #4

Peng

6 years, 11 months ago (2014-01-08 21:48:36 UTC) #5

https://codereview.chromium.org/126823007/diff/150001/ppapi/c/ppb_media_strea...
File ppapi/c/ppb_media_stream_video_track.h (right):

https://codereview.chromium.org/126823007/diff/150001/ppapi/c/ppb_media_strea...
ppapi/c/ppb_media_stream_video_track.h:22: * Defines the
<code>PPB_MediaStreamVideoTrack</code> interface. Used for
On 2014/01/08 19:01:45, yzshen1 wrote:
> Why this is removed? I think this comment is useful. Is it some issue in the
> generator?
Probably it is because I add [generate_trunk] at a wrong place.
Done.

https://codereview.chromium.org/126823007/diff/150001/ppapi/thunk/ppb_media_s...
File ppapi/thunk/ppb_media_stream_video_track_api.h (right):

https://codereview.chromium.org/126823007/diff/150001/ppapi/thunk/ppb_media_s...
ppapi/thunk/ppb_media_stream_video_track_api.h:13: class
PPB_MediaStreamVideoTrack_API : virtual public PPB_MediaStreamTrack_API{
As we discussed offline. Only the MediaStreamBaseShared will be renamed to
MediaStreamFrameBuffer and used as a member field. And it is OK to keep
MediaStreamTrackResourceBase & 
MediaStreamTrackHostBase.

Done


On 2014/01/08 19:01:45, yzshen1 wrote:
> As I comment in the other CL. I think the MediaStreamTrackResourceBase should
be
> named MediaFrameBuffer and it should a member of other classes instead of a
base
> class.
> 
> This simplifies inheritance hierarchy. And it also gives us more flexibility,
> for example, when frame size changes, the Track can simply create a new
> MediaFrameBuffer member. 
> 
> .*_APIs should be pure interfaces. And I believe virtual inheritance is not
> recommended. :)
> 
> On 2014/01/08 18:51:26, Peng wrote:
> > On 2014/01/08 17:56:11, yzshen1 wrote:
> > > - You don't need virtual inheritance here, right?
> > > - one space before '{'.
> > 
> > I would like to implement some methods of PPB_MediaStreamTrack_API in the
> > MediaStreamTrackResourceBase.
> > And the MediaStreamVideoTrackResource will inherit from both
> > MediaStreamTrackResourceBase & PPB_MediaStreamVideoTrack_API. In that case,
i
> > think virtual inheritance is needed here, right?
>

https://codereview.chromium.org/126823007/diff/230001/ppapi/c/ppb_video_frame.h
File ppapi/c/ppb_video_frame.h (left):

https://codereview.chromium.org/126823007/diff/230001/ppapi/c/ppb_video_frame...
ppapi/c/ppb_video_frame.h:21: * Defines the <code>PPB_VideoFrame</code>
interface.
On 2014/01/08 19:01:45, yzshen1 wrote:
> I think this must be some issue in the generator?
Done

https://codereview.chromium.org/126823007/diff/230001/ppapi/thunk/ppb_media_s...
File ppapi/thunk/ppb_media_stream_video_track_thunk.cc (right):

https://codereview.chromium.org/126823007/diff/230001/ppapi/thunk/ppb_media_s...
ppapi/thunk/ppb_media_stream_video_track_thunk.cc:48: return PP_FALSE;
On 2014/01/08 19:01:45, yzshen1 wrote:
> Maybe you should use on-failure tag to override this?

Done.

bbudge

https://codereview.chromium.org/126823007/diff/660001/ppapi/thunk/ppb_media_stream_video_track_api.h File ppapi/thunk/ppb_media_stream_video_track_api.h (right): https://codereview.chromium.org/126823007/diff/660001/ppapi/thunk/ppb_media_stream_video_track_api.h#newcode5 ppapi/thunk/ppb_media_stream_video_track_api.h:5: #ifndef PPAPI_THUNK_MEDIA_STREAM_VIDEO_TRACK_API_H_ should be PPAPI_THUNK_PPB_MEDIA_STREAM_VIDEO_TRACK_API_H_

6 years, 11 months ago (2014-01-09 01:35:33 UTC) #6

Peng

6 years, 11 months ago (2014-01-09 02:06:51 UTC) #7

bbudge

https://codereview.chromium.org/126823007/diff/800001/ppapi/api/ppb_media_stream_video_track.idl File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/126823007/diff/800001/ppapi/api/ppb_media_stream_video_track.idl#newcode82 ppapi/api/ppb_media_stream_video_track.idl:82: * <code>callback</code> will be called, when a new frame ...

6 years, 11 months ago (2014-01-09 18:55:26 UTC) #8

Peng

CL is updated. PTAL. Thanks. https://codereview.chromium.org/126823007/diff/800001/ppapi/api/ppb_media_stream_video_track.idl File ppapi/api/ppb_media_stream_video_track.idl (right): https://codereview.chromium.org/126823007/diff/800001/ppapi/api/ppb_media_stream_video_track.idl#newcode82 ppapi/api/ppb_media_stream_video_track.idl:82: * <code>callback</code> will be ...

6 years, 11 months ago (2014-01-09 19:09:38 UTC) #9

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/126823007/1170001

6 years, 11 months ago (2014-01-10 00:08:07 UTC) #13

Message was sent while issue was closed.

Change committed as 244065

Expand Messages | Collapse Messages