Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
212533007:
Sync NaCl MediaStream Video example with ppapi/examples/media_stream_video (Closed)
|
Created:
6 years, 9 months ago by Peng Modified:
5 years, 1 month ago CC:
chromium-reviews, binji, Sam Clegg, Ronghua Wu (Left Chromium), kmixter1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSync NaCl MediaStream Video example with ppapi/examples/media_stream_video
It also fixes a bug in release build.
BUG=330851
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261748
Patch Set 1 #Patch Set 2 : Update #Patch Set 3 : Update #
Total comments: 13
Patch Set 4 : Fix review issues #Patch Set 5 : Rebase #
Total comments: 3
Patch Set 6 : Fix review issue #
Messages
Total messages: 32 (4 generated)
Hi Ben, PTAL. Thanks.
+sbc; binji is out of the office ATM
https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... File native_client_sdk/src/examples/api/media_stream_video/index.html (right): https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... native_client_sdk/src/examples/api/media_stream_video/index.html:28: <button onclick="changeFormat('I420')" >I420</button> Doesn't CSP make inline JavaScript illegal? I think we need all our example to be CSP compliant so that we can build them into a packaged app. https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... File native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc (right): https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:105: const struct PPB_OpenGLES2* gles2_if_; Why not just use -lgles2_ppapi and use the GLES2 API directly? I would make this code more readable and portable. https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:154: if (!var_message.is_dictionary()) Report this case as an error somehow (even just stderr)? https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:183: } else { error report in some way } https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:378: kVertexShader, sizeof(kVertexShader)); Maybe don't pass size here and assume NULL terminated strings? Would make this code easier to read I think as all statements would fix on a single line?
https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... File native_client_sdk/src/examples/api/media_stream_video/index.html (right): https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... native_client_sdk/src/examples/api/media_stream_video/index.html:28: <button onclick="changeFormat('I420')" >I420</button> On 2014/03/28 18:24:56, Sam Clegg wrote: > Doesn't CSP make inline JavaScript illegal? I think we need all our example to > be CSP compliant so that we can build them into a packaged app. Yes, this is correct. We typically do this sort of behavior in an "attachListeners" function. It will be automatically called by common.js. See core/example.js, for example. https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... File native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc (right): https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:168: std::string str_format = var_dictionary_message.Get("format").AsString(); nit: extra space after =
Cl has been updated. PTAL. Thanks. https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... File native_client_sdk/src/examples/api/media_stream_video/index.html (right): https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... native_client_sdk/src/examples/api/media_stream_video/index.html:28: <button onclick="changeFormat('I420')" >I420</button> On 2014/04/01 00:00:52, binji wrote: > On 2014/03/28 18:24:56, Sam Clegg wrote: > > Doesn't CSP make inline JavaScript illegal? I think we need all our example > to > > be CSP compliant so that we can build them into a packaged app. > > Yes, this is correct. We typically do this sort of behavior in an > "attachListeners" function. It will be automatically called by common.js. See > core/example.js, for example. Done. https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... File native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc (right): https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:105: const struct PPB_OpenGLES2* gles2_if_; On 2014/03/28 18:24:56, Sam Clegg wrote: > Why not just use -lgles2_ppapi and use the GLES2 API directly? I would make > this code more readable and portable. Done. https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:154: if (!var_message.is_dictionary()) On 2014/03/28 18:24:56, Sam Clegg wrote: > Report this case as an error somehow (even just stderr)? Done. https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:168: std::string str_format = var_dictionary_message.Get("format").AsString(); On 2014/04/01 00:00:52, binji wrote: > nit: extra space after = Done. https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:183: } On 2014/03/28 18:24:56, Sam Clegg wrote: > else { error report in some way } Done. https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:378: kVertexShader, sizeof(kVertexShader)); On 2014/03/28 18:24:56, Sam Clegg wrote: > Maybe don't pass size here and assume NULL terminated strings? > > Would make this code easier to read I think as all statements > would fix on a single line? Done.
On 2014/04/01 15:36:37, Peng wrote: > Cl has been updated. PTAL. Thanks. > > https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... > File native_client_sdk/src/examples/api/media_stream_video/index.html (right): > > https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... > native_client_sdk/src/examples/api/media_stream_video/index.html:28: <button > onclick="changeFormat('I420')" >I420</button> > On 2014/04/01 00:00:52, binji wrote: > > On 2014/03/28 18:24:56, Sam Clegg wrote: > > > Doesn't CSP make inline JavaScript illegal? I think we need all our example > > to > > > be CSP compliant so that we can build them into a packaged app. > > > > Yes, this is correct. We typically do this sort of behavior in an > > "attachListeners" function. It will be automatically called by common.js. See > > core/example.js, for example. > > Done. > > https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... > File native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc > (right): > > https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... > native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:105: > const struct PPB_OpenGLES2* gles2_if_; > On 2014/03/28 18:24:56, Sam Clegg wrote: > > Why not just use -lgles2_ppapi and use the GLES2 API directly? I would make > > this code more readable and portable. > > Done. > > https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... > native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:154: > if (!var_message.is_dictionary()) > On 2014/03/28 18:24:56, Sam Clegg wrote: > > Report this case as an error somehow (even just stderr)? > > Done. > > https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... > native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:168: > std::string str_format = var_dictionary_message.Get("format").AsString(); > On 2014/04/01 00:00:52, binji wrote: > > nit: extra space after = > > Done. > > https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... > native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:183: > } > On 2014/03/28 18:24:56, Sam Clegg wrote: > > else { error report in some way } > > Done. > > https://codereview.chromium.org/212533007/diff/40001/native_client_sdk/src/ex... > native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:378: > kVertexShader, sizeof(kVertexShader)); > On 2014/03/28 18:24:56, Sam Clegg wrote: > > Maybe don't pass size here and assume NULL terminated strings? > > > > Would make this code easier to read I think as all statements > > would fix on a single line? > > Done. Kindly ping.
Great, much easier to read! slgtm
On 2014/04/03 18:02:20, binji wrote: > Great, much easier to read! slgtm David, the latest patchset changes code in ppapi too. PTAL. Thanks.
lgtm
lgtm https://codereview.chromium.org/212533007/diff/100001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/media_stream_video/index.html (right): https://codereview.chromium.org/212533007/diff/100001/native_client_sdk/src/e... native_client_sdk/src/examples/api/media_stream_video/index.html:22: or with PNaCL.<br> Why wont it work on the store? Is it just a temporary thing? Perhaps mention the reason here.
https://codereview.chromium.org/212533007/diff/100001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/media_stream_video/index.html (right): https://codereview.chromium.org/212533007/diff/100001/native_client_sdk/src/e... native_client_sdk/src/examples/api/media_stream_video/index.html:22: or with PNaCL.<br> On 2014/04/03 18:24:17, Sam Clegg wrote: > Why wont it work on the store? Is it just a temporary thing? Perhaps mention > the reason here. Oops, that's probably a merge mistake. This line was there previously, but I removed it when I pulled the APIs to stable. (Unfortunately going to stable just missed 35 :( ) In any case, we should remove that line now. Good eye.
https://codereview.chromium.org/212533007/diff/100001/native_client_sdk/src/e... File native_client_sdk/src/examples/api/media_stream_video/index.html (right): https://codereview.chromium.org/212533007/diff/100001/native_client_sdk/src/e... native_client_sdk/src/examples/api/media_stream_video/index.html:22: or with PNaCL.<br> On 2014/04/03 18:26:28, dmichael wrote: > On 2014/04/03 18:24:17, Sam Clegg wrote: > > Why wont it work on the store? Is it just a temporary thing? Perhaps mention > > the reason here. > Oops, that's probably a merge mistake. This line was there previously, but I > removed it when I pulled the APIs to stable. (Unfortunately going to stable just > missed 35 :( ) > > In any case, we should remove that line now. Good eye. Done
The CQ bit was checked by [email protected]
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/[email protected]/212533007/120001
The CQ bit was unchecked by [email protected]
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by [email protected]
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/[email protected]/212533007/120001
Message was sent while issue was closed.
Change committed as 261748
Message was sent while issue was closed.
[email protected] changed reviewers: + [email protected]
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
#fix11
Message was sent while issue was closed.
#fix11
Message was sent while issue was closed.
#fix11
Message was sent while issue was closed.
On 2015/08/14 15:21:54, riwsakiza55h wrote: > #fix11
Message was sent while issue was closed.
[email protected] changed reviewers: + [email protected]
Message was sent while issue was closed.
Message was sent while issue was closed.
[email protected] changed reviewers: + [email protected]
Message was sent while issue was closed.
Message was sent while issue was closed.
[email protected] changed reviewers: + [email protected] - [email protected]
Message was sent while issue was closed.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
