Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9956068:
Change macro definitions for android/arm and add srtp tests into gyp (Closed)
|
Created:
8 years, 8 months ago by leozwang Modified:
8 years, 8 months ago CC:
chromium-reviews Base URL:
https://src.chromium.org/svn/trunk/deps/ Visibility:
Public. |
DescriptionChange macro definitions for android/arm and add srtp tests into gyp
BUG=
TEST=
Patch Set 1 #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #
Total comments: 5
Patch Set 4 : #Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 5
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #Messages
Total messages: 26 (0 generated)
Add macro definitions for android/arm. These macro definitions are imported from AOSP. I keep the original macro definitions for non-android/arm platforms not to break any existing builds. Please review it.
I'm not sure what you need this change for. Doesn't libsrtp compile on android without these changes? http://codereview.chromium.org/9956068/diff/1/third_party/libsrtp/libsrtp.gyp File third_party/libsrtp/libsrtp.gyp (right): http://codereview.chromium.org/9956068/diff/1/third_party/libsrtp/libsrtp.gyp... third_party/libsrtp/libsrtp.gyp:42: 'CPU_CISC', This seems wrong to me. ARM is always RISC architecture, even when you run android on it :-) http://codereview.chromium.org/9956068/diff/1/third_party/libsrtp/libsrtp.gyp... third_party/libsrtp/libsrtp.gyp:44: 'HAVE_ARPA_INET_H', Do we need to have all these defines? Many of them seem to be common with other platforms so it would be wrong to redefine them specifically for android.
Let me briefly describe what I have done. I should have added some background information into issue description, sorry about it. I'm trying to make webrtc which includes libjingle which includes srtp work on android/arm. When I tested original chromium build, libjingle failed in srtp_init, more specific, in aes_icm self test. So I turned to AOSP, after I imported all these macro definitions, srtp started working on android. I talked to Mallinath about it, seems no one has tested srtp in chromium on arm, particularly on android, yet. My test also proved that setttings in AOSP are correct.
http://codereview.chromium.org/9956068/diff/1/third_party/libsrtp/libsrtp.gyp File third_party/libsrtp/libsrtp.gyp (right): http://codereview.chromium.org/9956068/diff/1/third_party/libsrtp/libsrtp.gyp... third_party/libsrtp/libsrtp.gyp:42: 'CPU_CISC', It seemed not a right flag to me at the beginning. srtp uses this flag to define different data on different platforms, particularly in aes.c. I'm not familiar with how srtp works internally, but this macro set seems work in my test, AOSP uses same macro defines and it works fine. I can dig into it if needed. http://codereview.chromium.org/9956068/diff/1/third_party/libsrtp/libsrtp.gyp... third_party/libsrtp/libsrtp.gyp:44: 'HAVE_ARPA_INET_H', I was also debating about if I should keep this long list. Because I don't have clear picture of what other platforms will be and what will be supported on other arm processor based platforms, I decided to keep this long list for android. If a defines is clearly redefined, I will remove from the list. Also, I decided to keep this long list because seems these defines are generated by configure script, I'm not sure if removing any of them could cause any potential problem. If there is a test or test set that can verify these flags, I can do more test to make this list shorter.
I compared config.h which is created on linux with android macro defines, I think I might be able to remove some macro defines on android. However, how do I know what I'm doing is correct (it will not cause problem in future)? What tests do you guys normally run after you change macro defines? I can follow the same procedure and upload a new patch.
On 2012/04/02 22:03:18, leozwang wrote: > I compared config.h which is created on linux with android macro defines, I > think I might be able to remove some macro defines on android. > However, how do I know what I'm doing is correct (it will not cause problem in > future)? What tests do you guys normally run after you change macro defines? I > can follow the same procedure and upload a new patch. Leo, Until now we haven't enabled test cases in libsrtp. Since it is a third party library and not modified anything on top of what we downloaded, there was no need to enable these tests earlier. If you think we should have tests to help future platform compilations, then we should do it before you commit your CL. There are some tests available as part of the source, we should define a target. I might have some bandwidth later this week to enable these tests. Let me know.
I don't think it makes sense to separate Android ARM build from other ARM configurations. Given that nobody ever tested webrtc on ARM anyway, you don't need to worry about breaking it. You can use the cros_arm trybots to verify that you are not breaking the build. http://codereview.chromium.org/9956068/diff/1/third_party/libsrtp/libsrtp.gyp File third_party/libsrtp/libsrtp.gyp (right): http://codereview.chromium.org/9956068/diff/1/third_party/libsrtp/libsrtp.gyp... third_party/libsrtp/libsrtp.gyp:42: 'CPU_CISC', On 2012/04/02 21:14:57, leozwang wrote: > It seemed not a right flag to me at the beginning. srtp uses this flag to define > different data on different platforms, particularly in aes.c. I'm not familiar > with how srtp works internally, but this macro set seems work in my test, AOSP > uses same macro defines and it works fine. I can dig into it if needed. As far as I understand libsrtp uses that to different AES implementations depending on CPU architecture - one algorithm works better RISC, and the other one - on CISC. So even if you use CPU_CISC on ARM I would expect it would still compile, but would be slower. It's also possible that the RISC implementation is broken because not many people use it, and that's the reason it didn't work for you. It shouldn't be hard to verify. If that's the problem then we will need to track down the issue and fix it in libsrtp, but it's OK to keep CPU_CISC here. http://codereview.chromium.org/9956068/diff/1/third_party/libsrtp/libsrtp.gyp... third_party/libsrtp/libsrtp.gyp:44: 'HAVE_ARPA_INET_H', On 2012/04/02 21:14:57, leozwang wrote: > I was also debating about if I should keep this long list. Because I don't have > clear picture of what other platforms will be and what will be supported on > other arm processor based platforms, I decided to keep this long list for > android. If a defines is clearly redefined, I will remove from the list. > Also, I decided to keep this long list because seems these defines are generated > by configure script, I'm not sure if removing any of them could cause any > potential problem. If there is a test or test set that can verify these flags, I > can do more test to make this list shorter. I don't think any of these can cause any problems as long as it compiles. Please try to find out of CPU_CISC/CPU_RISC is what causing the problem.
That would be great. When I worked on it, I didn't have a small test set, but I was lucky that AOSP provides a working version I can take reference. Please let me know if I can help you on creating srtp test apps.
> I don't think any of these can cause any problems as long as it compiles. Please > try to find out of CPU_CISC/CPU_RISC is what causing the problem. Sure, I will try to combine effort with building srtp test app as Mallinath mentioned. Thanks.
I uploaded a new patch set 3, please review it, there are 2 major changes in it 1. Redefined macro defines for arm. Please see my comments in source file. 2. I added srtp test application targets in gyp. Developer can now do "make srtp_runtest" to build all test applications and run them on target platform. http://codereview.chromium.org/9956068/diff/4003/third_party/libsrtp/libsrtp.gyp File third_party/libsrtp/libsrtp.gyp (right): http://codereview.chromium.org/9956068/diff/4003/third_party/libsrtp/libsrtp.... third_party/libsrtp/libsrtp.gyp:33: 'CPU_CISC', CPU_CISC is used for arm now. CPU_RISC is the cause that makes srtp fail, it also causes srtp test app fail. I can dig into it if its needed. I accidentally found this link, https://trac.pjsip.org/repos/changeset/1746/pjproject/trunk/third_party/build.... which means CPU_RISC didn't work well before, seems its still broken now. http://codereview.chromium.org/9956068/diff/4003/third_party/libsrtp/libsrtp.... third_party/libsrtp/libsrtp.gyp:40: 'HAVE_UINT8_T', I have to add these defines to make it compile on android. (It might not be necessary on other arm platforms.) If I don't have these defines, I will get, for example, "uint16_t redefined" error. The reason why they're needed only for android is because android has slight different libc implementation, these data type are already defined by android, if application defines these data type again, it will cause compilation error. The details, for example in srtp integer.h typedef unsigned short int uint16_t On android: in _type.h typedef unsigned short int __uint16_t <- which is included by stdint.h in stdint.h typedef __uint16_t uint16_t <- which is included by stdlib.h In order to remove compilation error, I added these 7 macro defines.
http://codereview.chromium.org/9956068/diff/4003/third_party/libsrtp/libsrtp.gyp File third_party/libsrtp/libsrtp.gyp (right): http://codereview.chromium.org/9956068/diff/4003/third_party/libsrtp/libsrtp.... third_party/libsrtp/libsrtp.gyp:254: 'target_name': 'libcryptomodule', Why do you need one more target, compiling same files?
http://codereview.chromium.org/9956068/diff/4003/third_party/libsrtp/libsrtp.gyp File third_party/libsrtp/libsrtp.gyp (right): http://codereview.chromium.org/9956068/diff/4003/third_party/libsrtp/libsrtp.... third_party/libsrtp/libsrtp.gyp:254: 'target_name': 'libcryptomodule', One reason is its from original makefile, another reason is the list of source file is slightly different.
Hi Sergey, will you please take a look at this CL?
http://codereview.chromium.org/9956068/diff/4003/third_party/libsrtp/libsrtp.gyp File third_party/libsrtp/libsrtp.gyp (right): http://codereview.chromium.org/9956068/diff/4003/third_party/libsrtp/libsrtp.... third_party/libsrtp/libsrtp.gyp:254: 'target_name': 'libcryptomodule', I would prefer having the single target. If there is change in the source, then can you add those files to the main target. Otherwise we are testing two different things. On 2012/04/04 16:53:11, leozwang wrote: > One reason is its from original makefile, another reason is the list of source > file is slightly different.
In patch set 4, srtp/crypto/ae_xfm/xfm.c is added to libsrtp, all test apps depend on libsrtp now. Mallinath, please take a look.
It turned out that xfm.c is not used anywhere, so remove it from gyp. please review it.
Ping again, Please kindly let me know what else I missed so far, I think I have addressed all issues. It's a bug and blocker that blocking my chromium/android development, I want to solve this problem asap. Please let me know what I should do on my side. thanks a lot!
Sorry for delay. Next time don't hesitate to ping me in gtalk when I'm so unresponsive. LGTM if my comments are addressed. http://codereview.chromium.org/9956068/diff/14001/third_party/libsrtp/libsrtp... File third_party/libsrtp/libsrtp.gyp (right): http://codereview.chromium.org/9956068/diff/14001/third_party/libsrtp/libsrtp... third_party/libsrtp/libsrtp.gyp:33: 'CPU_CISC', This would be very confusing for anybody looking at this code. Please add comment explaining that CPU_RISK is not working properly now for unknown reasons. Also add TODO to investigate and fix the issue and change this to CPU_RISK. http://codereview.chromium.org/9956068/diff/14001/third_party/libsrtp/libsrtp... third_party/libsrtp/libsrtp.gyp:82: 'CPU_CISC', Don't think we need this in direct_dependent_settings. It's not used anywhere in libsrtp headers, and may conflict with other code.
lgtm. On 2012/04/09 17:54:31, sergeyu wrote: > Sorry for delay. Next time don't hesitate to ping me in gtalk when I'm so > unresponsive. > > LGTM if my comments are addressed. > > http://codereview.chromium.org/9956068/diff/14001/third_party/libsrtp/libsrtp... > File third_party/libsrtp/libsrtp.gyp (right): > > http://codereview.chromium.org/9956068/diff/14001/third_party/libsrtp/libsrtp... > third_party/libsrtp/libsrtp.gyp:33: 'CPU_CISC', > This would be very confusing for anybody looking at this code. Please add > comment explaining that CPU_RISK is not working properly now for unknown > reasons. Also add TODO to investigate and fix the issue and change this to > CPU_RISK. > > http://codereview.chromium.org/9956068/diff/14001/third_party/libsrtp/libsrtp... > third_party/libsrtp/libsrtp.gyp:82: 'CPU_CISC', > Don't think we need this in direct_dependent_settings. It's not used anywhere in > libsrtp headers, and may conflict with other code.
Hi Serge, your comments have been addressed in patch set 6, please take a look. linux_chromeos trybot failed in ui_test, I think we can ignore it for now. http://codereview.chromium.org/9956068/diff/14001/third_party/libsrtp/libsrtp... File third_party/libsrtp/libsrtp.gyp (right): http://codereview.chromium.org/9956068/diff/14001/third_party/libsrtp/libsrtp... third_party/libsrtp/libsrtp.gyp:33: 'CPU_CISC', On 2012/04/09 17:54:32, sergeyu wrote: > This would be very confusing for anybody looking at this code. Please add > comment explaining that CPU_RISK is not working properly now for unknown > reasons. Also add TODO to investigate and fix the issue and change this to > CPU_RISK. Done. http://codereview.chromium.org/9956068/diff/14001/third_party/libsrtp/libsrtp... third_party/libsrtp/libsrtp.gyp:82: 'CPU_CISC', On 2012/04/09 17:54:32, sergeyu wrote: > Don't think we need this in direct_dependent_settings. It's not used anywhere in > libsrtp headers, and may conflict with other code. Done.
Couple more nits. The ui_tests failure on Chrome OS must be flake. http://codereview.chromium.org/9956068/diff/22001/third_party/libsrtp/libsrtp... File third_party/libsrtp/libsrtp.gyp (right): http://codereview.chromium.org/9956068/diff/22001/third_party/libsrtp/libsrtp... third_party/libsrtp/libsrtp.gyp:33: # TODO(leozwang): CPU_RISC doesn't work properly on android/arm platform nit: 80-chars lines. http://codereview.chromium.org/9956068/diff/22001/third_party/libsrtp/libsrtp... third_party/libsrtp/libsrtp.gyp:34: # for unknown reasons, need to investigate the root cause of it. Also explain that this is used for optimization only, and CPU_CISC should work just fine.
Hi Serge, please check the new patch and wording. http://codereview.chromium.org/9956068/diff/22001/third_party/libsrtp/libsrtp... File third_party/libsrtp/libsrtp.gyp (right): http://codereview.chromium.org/9956068/diff/22001/third_party/libsrtp/libsrtp... third_party/libsrtp/libsrtp.gyp:33: # TODO(leozwang): CPU_RISC doesn't work properly on android/arm platform On 2012/04/09 18:56:22, sergeyu wrote: > nit: 80-chars lines. Done. http://codereview.chromium.org/9956068/diff/22001/third_party/libsrtp/libsrtp... third_party/libsrtp/libsrtp.gyp:33: # TODO(leozwang): CPU_RISC doesn't work properly on android/arm platform On 2012/04/09 18:56:22, sergeyu wrote: > nit: 80-chars lines. sorry http://codereview.chromium.org/9956068/diff/22001/third_party/libsrtp/libsrtp... third_party/libsrtp/libsrtp.gyp:34: # for unknown reasons, need to investigate the root cause of it. On 2012/04/09 18:56:22, sergeyu wrote: > Also explain that this is used for optimization only, and CPU_CISC should work > just fine. Done.
lgtm http://codereview.chromium.org/9956068/diff/24002/third_party/libsrtp/libsrtp... File third_party/libsrtp/libsrtp.gyp (right): http://codereview.chromium.org/9956068/diff/24002/third_party/libsrtp/libsrtp... third_party/libsrtp/libsrtp.gyp:370: print " (srtp_test_aes_calc output should be 69c4e0d86a7b0430d8cdb78070b4c55a)"; \ can you wrap this same as the previous line?
Patch set 8 is uploaded, hopefully it's the last one. thanks. http://codereview.chromium.org/9956068/diff/24002/third_party/libsrtp/libsrtp... File third_party/libsrtp/libsrtp.gyp (right): http://codereview.chromium.org/9956068/diff/24002/third_party/libsrtp/libsrtp... third_party/libsrtp/libsrtp.gyp:370: print " (srtp_test_aes_calc output should be 69c4e0d86a7b0430d8cdb78070b4c55a)"; \ On 2012/04/09 19:52:53, sergeyu wrote: > can you wrap this same as the previous line? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leozwang@google.com/9956068/25001
Change committed as 131459 |
