Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1819553002:
Added the JNI interface to get and set RtpParameters and the maximum bitrate limits. (Closed)
DescriptionAdded the JNI interface to get and set RtpParameters and the maximum bitrate limits.
Defined a JavaCollection convenience class to simplify iterating over collections from within JNI code
Follow-up to https://codereview.webrtc.org/1788583004/.
BUG=
Committed: https://crrev.com/303b3c21a4adf67359734b1bfbf5bdb9b25ad6c7
Cr-Commit-Position: refs/heads/master@{#12125}
Patch Set 1 #Patch Set 2 : Removed the dependency on the rtc::Optional CL #
Total comments: 12
Patch Set 3 : Addressed code review feedback; moved the implementation for Iterator out of the header file as it … #Patch Set 4 : One more rename #
Total comments: 15
Patch Set 5 : Addressed some more code review feedback. #
Total comments: 21
Patch Set 6 : Code review feedback #
Messages
Total messages: 28 (10 generated)
skvlad@webrtc.org changed reviewers: + deadbeef@webrtc.org, magjed@webrtc.org
This change adds a Java API for reading and changing RtpParameters (http://w3c.github.io/webrtc-pc/#dictionary-rtcrtpparameters-members). This change assumes the rtc::Optional<int> change is present (https://codereview.webrtc.org/1813763005/). If you aren't comfortable with the JavaCollection class change, I have a version that doesn't include it - at the cost of duplicating the JNI iteration code one more time.
I have removed the dependency on the rtc::Optional CL - this change treats bitrate <= 0 as "not set". When the rtc::Optional CL lands, the Java API will not have to change.
pthatcher@webrtc.org changed reviewers: + pthatcher@webrtc.org
Just names https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/jni/jni_h... File webrtc/api/java/jni/jni_helpers.h (right): https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:132: class JavaCollectionIterator { It would actually be JavaIterableIterator. How about just webrtc_jni::Iterator? https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:175: bool have_next = jni_->CallBooleanMethod(iterator_, has_next_id_); have_next => has_next https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:207: class JavaCollection { webrtc_jni::Iterable? https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:210: explicit JavaCollection(JNIEnv* jni, jobject collection) collection => iterable https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:219: jobject collection_; iterable_ https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/src/org/w... File webrtc/api/java/src/org/webrtc/RtpParameters.java (right): https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/RtpParameters.java:21: public static class RtpEncodingParameters { public Integer maxBitrateBps; } Maybe RtpParmaeters.Encoding?
Addressed code review feedback and moved the implementation of Iterator out of the header file. https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/jni/jni_h... File webrtc/api/java/jni/jni_helpers.h (right): https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:132: class JavaCollectionIterator { On 2016/03/21 21:57:00, pthatcher1 wrote: > It would actually be JavaIterableIterator. > > How about just webrtc_jni::Iterator? Done. https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:175: bool have_next = jni_->CallBooleanMethod(iterator_, has_next_id_); On 2016/03/21 21:57:00, pthatcher1 wrote: > have_next => has_next Done. https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:207: class JavaCollection { On 2016/03/21 21:57:01, pthatcher1 wrote: > webrtc_jni::Iterable? Done. https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:210: explicit JavaCollection(JNIEnv* jni, jobject collection) On 2016/03/21 21:57:01, pthatcher1 wrote: > collection => iterable Done. https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:219: jobject collection_; On 2016/03/21 21:57:00, pthatcher1 wrote: > iterable_ Done. https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/src/org/w... File webrtc/api/java/src/org/webrtc/RtpParameters.java (right): https://codereview.webrtc.org/1819553002/diff/20001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/RtpParameters.java:21: public static class RtpEncodingParameters { public Integer maxBitrateBps; } On 2016/03/21 21:57:01, pthatcher1 wrote: > Maybe RtpParmaeters.Encoding? Done.
Description was changed from ========== Added the JNI interface to get and set RtpParameters and the maximum bitrate limits. Defined a JavaCollection convenience class to simplify iterating over collections from within JNI code Follow-up to https://codereview.webrtc.org/1788583004/; assumes https://codereview.webrtc.org/1813763005/ is available but can be changed to work without that one. BUG= ========== to ========== Added the JNI interface to get and set RtpParameters and the maximum bitrate limits. Defined a JavaCollection convenience class to simplify iterating over collections from within JNI code Follow-up to https://codereview.webrtc.org/1788583004/; assumes https://codereview.webrtc.org/1813763005/ is available but can be changed to work without that one. BUG= ==========
skvlad@webrtc.org changed reviewers: + glaznev@webrtc.org
Added Alex as a reviewer. Alex, please take a look at the JNI code whenever you have time. Thanks!
Added Alex as a reviewer. Alex, please take a look at the JNI code whenever you have time. Thanks!
Looks good; mainly just style nits. Thanks for adding the Iterator class, that will definitely save a lot of typing in the future. https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/androidtests/s... File webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java (right): https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java:700: if (sender.track().kind().equals("video")) { nit: indentation. Unfortunately our Java code doesn't match the Chromium style guide exactly; we use 2-space indentation. https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java:713: // Verify that we can read back the updated value nit: Period at end of comment https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/jni/jni_h... File webrtc/api/java/jni/jni_helpers.cc (right): https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.cc:287: // TODO(skvlad): Should this be an exception instead? That seems reasonable. Just use "RTC_CHECK(iterator_ != NULL)" https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/jni/jni_h... File webrtc/api/java/jni/jni_helpers.h (right): https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:140: // functions nit: period at end of comments (in cc file too) https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/jni/peerc... File webrtc/api/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/jni/peerc... webrtc/api/java/jni/peerconnection_jni.cc:2084: CHECK_EXCEPTION(jni) << "error during CallBooleanMethod"; I don't think this CHECK_EXCEPTION is necessary. https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/src/org/w... File webrtc/api/java/src/org/webrtc/RtpParameters.java (right): https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/RtpParameters.java:21: public static class Encoding { public Integer maxBitrateBps; } nit: indentation https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/src/org/w... File webrtc/api/java/src/org/webrtc/RtpSender.java (right): https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/RtpSender.java:50: return nativeSetParameters(nativeRtpSender, parameters); nit: indentation https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/RtpSender.java:75: private static native boolean nativeSetParameters(long nativeRtpSender, RtpParameters parameters); nit: Line width
Addressed some more code review comments. https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/androidtests/s... File webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java (right): https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java:700: if (sender.track().kind().equals("video")) { On 2016/03/22 01:16:36, Taylor Brandstetter wrote: > nit: indentation. > Unfortunately our Java code doesn't match the Chromium style guide exactly; we > use 2-space indentation. Done. https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/PeerConnectionTest.java:713: // Verify that we can read back the updated value On 2016/03/22 01:16:36, Taylor Brandstetter wrote: > nit: Period at end of comment Done. https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/jni/jni_h... File webrtc/api/java/jni/jni_helpers.cc (right): https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.cc:287: // TODO(skvlad): Should this be an exception instead? On 2016/03/22 01:16:36, Taylor Brandstetter wrote: > That seems reasonable. Just use "RTC_CHECK(iterator_ != NULL)" Done. https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/jni/jni_h... File webrtc/api/java/jni/jni_helpers.h (right): https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:140: // functions On 2016/03/22 01:16:36, Taylor Brandstetter wrote: > nit: period at end of comments (in cc file too) Done. https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/jni/peerc... File webrtc/api/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/jni/peerc... webrtc/api/java/jni/peerconnection_jni.cc:2084: CHECK_EXCEPTION(jni) << "error during CallBooleanMethod"; On 2016/03/22 01:16:36, Taylor Brandstetter wrote: > I don't think this CHECK_EXCEPTION is necessary. Good catch. This is a leftover from the previous version that used iterators explicitly. The Iterable class does the check automatically now. I've fixed the other two uses as well. https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/src/org/w... File webrtc/api/java/src/org/webrtc/RtpSender.java (right): https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/RtpSender.java:50: return nativeSetParameters(nativeRtpSender, parameters); On 2016/03/22 01:16:36, Taylor Brandstetter wrote: > nit: indentation Done. https://codereview.webrtc.org/1819553002/diff/60001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/RtpSender.java:75: private static native boolean nativeSetParameters(long nativeRtpSender, RtpParameters parameters); On 2016/03/22 01:16:36, Taylor Brandstetter wrote: > nit: Line width Done.
https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... File webrtc/api/java/jni/jni_helpers.cc (right): https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.cc:276: Iterator::Iterator() : iterator_(NULL) {} nit: Use nullptr instead of NULL. This file seems to use both NULL and nullptr, but we should probably try to use nullptr. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.cc:302: next_id_(std::move(other.next_id_)){}; nit: space between '(' and '{'. Can you use 'git cl format' on this file? https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.cc:312: if (!has_next) { You should probably set |value_| to nullptr here as well. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... File webrtc/api/java/jni/jni_helpers.h (right): https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:132: class Iterator { Maybe this should be a nested class in Iterable instead? https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:150: return (iterator_ == other.iterator_); This looks a bit hacky, because you can't compare jobjects this way. I guess you only care about comparing with the end() iterator, but it would be nice if e.g. begin() == begin() reliably. Maybe implement this function with something like this: if (IsNull(jni, iterator_) || IsNull(jni, other.iterator_)) return IsNull(jni, iterator_) && IsNull(jni, other.iterator_); return jni->IsSameObject(value_, other.value_); https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:168: explicit Iterable(JNIEnv* jni, jobject iterable) Why explicit if you have two arguments? https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:169: : jni_(jni), iterable_(iterable) {} If you store JNIEnv, I would like to have a thread checker (webrtc/base/thread_checker.h) to make sure begin() is called on the same thread as the ctor. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/peerc... File webrtc/api/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/peerc... webrtc/api/java/jni/peerconnection_jni.cc:2064: jfieldID bitrate_id = GetFieldID(jni, j_encoding_parameters_class, Can you move |j_encoding_parameters_class| and |bitrate_id| outside the for loop to avoid creating them more than once? https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/peerc... webrtc/api/java/jni/peerconnection_jni.cc:2071: jmethodID integer_int_value_id = Move |integer_int_value_id| outside for loop. Strange name btw, why not just |int_value_id|? https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/peerc... webrtc/api/java/jni/peerconnection_jni.cc:2091: jclass parameters_class = FindClass(jni, "org/webrtc/RtpParameters"); If you are not planning to store this jclass, then I prefer if you use jni->FindClass("org/webrtc/RtpParameters") instead and remove "org/webrtc/RtpParameters" from classreferenceholder. Same goes for "org/webrtc/RtpParameters$Encoding" and "java/lang/Integer".
Corrected some code review issues - now using nullptr instead of NULL, made Iterator be a nested class of Iterable, removed references from ClassReferenceHolder, and added thread checks. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... File webrtc/api/java/jni/jni_helpers.cc (right): https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.cc:276: Iterator::Iterator() : iterator_(NULL) {} On 2016/03/22 15:15:30, magjed_webrtc wrote: > nit: Use nullptr instead of NULL. This file seems to use both NULL and nullptr, > but we should probably try to use nullptr. Thanks. I've updated the entire file to use nullptr. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.cc:302: next_id_(std::move(other.next_id_)){}; On 2016/03/22 15:15:30, magjed_webrtc wrote: > nit: space between '(' and '{'. Can you use 'git cl format' on this file? Done. Curiously, "git cl format" wants to remove this space when I add it. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.cc:312: if (!has_next) { On 2016/03/22 15:15:30, magjed_webrtc wrote: > You should probably set |value_| to nullptr here as well. Done. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... File webrtc/api/java/jni/jni_helpers.h (right): https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:132: class Iterator { On 2016/03/22 15:15:30, magjed_webrtc wrote: > Maybe this should be a nested class in Iterable instead? Done. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:150: return (iterator_ == other.iterator_); On 2016/03/22 15:15:30, magjed_webrtc wrote: > This looks a bit hacky, because you can't compare jobjects this way. I guess you > only care about comparing with the end() iterator, but it would be nice if e.g. > begin() == begin() reliably. Maybe implement this function with something like > this: > if (IsNull(jni, iterator_) || IsNull(jni, other.iterator_)) > return IsNull(jni, iterator_) && IsNull(jni, other.iterator_); > return jni->IsSameObject(value_, other.value_); This would make two iterators pointing to the same object in two different collections - or in two places in the same collection - be erroneously treated as equal. We can test that the two iterators point to the same collection, and that they have made the same number of steps - but even that won't work if they have been obtained at different times and the collection has been modified. Alternatively, we can have a special case for iterators pointing at begin() - which still won't solve the general case of comparing two iterators to the same collection. Even in standard C++, not all types of iterators can be compared for equality - for example, comparison for InputIterators (http://en.cppreference.com/w/cpp/concept/InputIterator) can be undefined. This iterator can be treated as a more restricted version of the input iterator that cannot be copied. In fact we can add a copy constructor and make it exactly satisfy the definition of an input iterator, but this will likely increase the chance of using it wrong without giving anything in exchange. For now I think the best we can do is to make the operation well defined for comparison with itself (*this == *this) and with the end() iterator - and undefined for anything else (checked with an RTC_DCHECK()). I've also documented this behavior in comments. What do you think? Is there something else we can do to keep it safe without adding too much complexity? https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:168: explicit Iterable(JNIEnv* jni, jobject iterable) On 2016/03/22 15:15:30, magjed_webrtc wrote: > Why explicit if you have two arguments? Oops, my mistake. It used to have one in the previous version of the change - before I realized I needed the JNIEnv. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:169: : jni_(jni), iterable_(iterable) {} On 2016/03/22 15:15:30, magjed_webrtc wrote: > If you store JNIEnv, I would like to have a thread checker > (webrtc/base/thread_checker.h) to make sure begin() is called on the same thread > as the ctor. Done. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/peerc... File webrtc/api/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/peerc... webrtc/api/java/jni/peerconnection_jni.cc:2064: jfieldID bitrate_id = GetFieldID(jni, j_encoding_parameters_class, On 2016/03/22 15:15:30, magjed_webrtc wrote: > Can you move |j_encoding_parameters_class| and |bitrate_id| outside the for loop > to avoid creating them more than once? Done. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/peerc... webrtc/api/java/jni/peerconnection_jni.cc:2071: jmethodID integer_int_value_id = On 2016/03/22 15:15:30, magjed_webrtc wrote: > Move |integer_int_value_id| outside for loop. Strange name btw, why not just > |int_value_id|? Done. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/peerc... webrtc/api/java/jni/peerconnection_jni.cc:2091: jclass parameters_class = FindClass(jni, "org/webrtc/RtpParameters"); On 2016/03/22 15:15:30, magjed_webrtc wrote: > If you are not planning to store this jclass, then I prefer if you use > jni->FindClass("org/webrtc/RtpParameters") instead and remove > "org/webrtc/RtpParameters" from classreferenceholder. Same goes for > "org/webrtc/RtpParameters$Encoding" and "java/lang/Integer". Done. I previously assumed ClassReferenceHolder was primarily used to speed up class lookups.
Corrected some code review issues - now using nullptr instead of NULL, made Iterator be a nested class of Iterable, removed references from ClassReferenceHolder, and added thread checks. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... File webrtc/api/java/jni/jni_helpers.cc (right): https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.cc:276: Iterator::Iterator() : iterator_(NULL) {} On 2016/03/22 15:15:30, magjed_webrtc wrote: > nit: Use nullptr instead of NULL. This file seems to use both NULL and nullptr, > but we should probably try to use nullptr. Thanks. I've updated the entire file to use nullptr. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.cc:302: next_id_(std::move(other.next_id_)){}; On 2016/03/22 15:15:30, magjed_webrtc wrote: > nit: space between '(' and '{'. Can you use 'git cl format' on this file? Done. Curiously, "git cl format" wants to remove this space when I add it. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.cc:312: if (!has_next) { On 2016/03/22 15:15:30, magjed_webrtc wrote: > You should probably set |value_| to nullptr here as well. Done. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... File webrtc/api/java/jni/jni_helpers.h (right): https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:132: class Iterator { On 2016/03/22 15:15:30, magjed_webrtc wrote: > Maybe this should be a nested class in Iterable instead? Done. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:150: return (iterator_ == other.iterator_); On 2016/03/22 15:15:30, magjed_webrtc wrote: > This looks a bit hacky, because you can't compare jobjects this way. I guess you > only care about comparing with the end() iterator, but it would be nice if e.g. > begin() == begin() reliably. Maybe implement this function with something like > this: > if (IsNull(jni, iterator_) || IsNull(jni, other.iterator_)) > return IsNull(jni, iterator_) && IsNull(jni, other.iterator_); > return jni->IsSameObject(value_, other.value_); This would make two iterators pointing to the same object in two different collections - or in two places in the same collection - be erroneously treated as equal. We can test that the two iterators point to the same collection, and that they have made the same number of steps - but even that won't work if they have been obtained at different times and the collection has been modified. Alternatively, we can have a special case for iterators pointing at begin() - which still won't solve the general case of comparing two iterators to the same collection. Even in standard C++, not all types of iterators can be compared for equality - for example, comparison for InputIterators (http://en.cppreference.com/w/cpp/concept/InputIterator) can be undefined. This iterator can be treated as a more restricted version of the input iterator that cannot be copied. In fact we can add a copy constructor and make it exactly satisfy the definition of an input iterator, but this will likely increase the chance of using it wrong without giving anything in exchange. For now I think the best we can do is to make the operation well defined for comparison with itself (*this == *this) and with the end() iterator - and undefined for anything else (checked with an RTC_DCHECK()). I've also documented this behavior in comments. What do you think? Is there something else we can do to keep it safe without adding too much complexity? https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:168: explicit Iterable(JNIEnv* jni, jobject iterable) On 2016/03/22 15:15:30, magjed_webrtc wrote: > Why explicit if you have two arguments? Oops, my mistake. It used to have one in the previous version of the change - before I realized I needed the JNIEnv. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:169: : jni_(jni), iterable_(iterable) {} On 2016/03/22 15:15:30, magjed_webrtc wrote: > If you store JNIEnv, I would like to have a thread checker > (webrtc/base/thread_checker.h) to make sure begin() is called on the same thread > as the ctor. Done. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/peerc... File webrtc/api/java/jni/peerconnection_jni.cc (right): https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/peerc... webrtc/api/java/jni/peerconnection_jni.cc:2064: jfieldID bitrate_id = GetFieldID(jni, j_encoding_parameters_class, On 2016/03/22 15:15:30, magjed_webrtc wrote: > Can you move |j_encoding_parameters_class| and |bitrate_id| outside the for loop > to avoid creating them more than once? Done. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/peerc... webrtc/api/java/jni/peerconnection_jni.cc:2071: jmethodID integer_int_value_id = On 2016/03/22 15:15:30, magjed_webrtc wrote: > Move |integer_int_value_id| outside for loop. Strange name btw, why not just > |int_value_id|? Done. https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/peerc... webrtc/api/java/jni/peerconnection_jni.cc:2091: jclass parameters_class = FindClass(jni, "org/webrtc/RtpParameters"); On 2016/03/22 15:15:30, magjed_webrtc wrote: > If you are not planning to store this jclass, then I prefer if you use > jni->FindClass("org/webrtc/RtpParameters") instead and remove > "org/webrtc/RtpParameters" from classreferenceholder. Same goes for > "org/webrtc/RtpParameters$Encoding" and "java/lang/Integer". Done. I previously assumed ClassReferenceHolder was primarily used to speed up class lookups.
webrtc/api/java/jni/* lgtm. The iterator over java collections is a big improvement, thanks for doing it! https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... File webrtc/api/java/jni/jni_helpers.h (right): https://codereview.webrtc.org/1819553002/diff/80001/webrtc/api/java/jni/jni_h... webrtc/api/java/jni/jni_helpers.h:150: return (iterator_ == other.iterator_); On 2016/03/23 00:29:27, skvlad wrote: > On 2016/03/22 15:15:30, magjed_webrtc wrote: > > This looks a bit hacky, because you can't compare jobjects this way. I guess > you > > only care about comparing with the end() iterator, but it would be nice if > e.g. > > begin() == begin() reliably. Maybe implement this function with something like > > this: > > if (IsNull(jni, iterator_) || IsNull(jni, other.iterator_)) > > return IsNull(jni, iterator_) && IsNull(jni, other.iterator_); > > return jni->IsSameObject(value_, other.value_); > This would make two iterators pointing to the same object in two different > collections - or in two places in the same collection - be erroneously treated > as equal. > We can test that the two iterators point to the same collection, and that they > have made the same number of steps - but even that won't work if they have been > obtained at different times and the collection has been modified. > Alternatively, we can have a special case for iterators pointing at begin() - > which still won't solve the general case of comparing two iterators to the same > collection. > Even in standard C++, not all types of iterators can be compared for equality - > for example, comparison for InputIterators > (http://en.cppreference.com/w/cpp/concept/InputIterator) can be undefined. This > iterator can be treated as a more restricted version of the input iterator that > cannot be copied. > In fact we can add a copy constructor and make it exactly satisfy the definition > of an input iterator, but this will likely increase the chance of using it wrong > without giving anything in exchange. > > For now I think the best we can do is to make the operation well defined for > comparison with itself (*this == *this) and with the end() iterator - and > undefined for anything else (checked with an RTC_DCHECK()). I've also documented > this behavior in comments. > What do you think? Is there something else we can do to keep it safe without > adding too much complexity? This is good, thanks for the detailed explanation!
lgtm
The CQ bit was checked by skvlad@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819553002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819553002/100001
Description was changed from ========== Added the JNI interface to get and set RtpParameters and the maximum bitrate limits. Defined a JavaCollection convenience class to simplify iterating over collections from within JNI code Follow-up to https://codereview.webrtc.org/1788583004/; assumes https://codereview.webrtc.org/1813763005/ is available but can be changed to work without that one. BUG= ========== to ========== Added the JNI interface to get and set RtpParameters and the maximum bitrate limits. Defined a JavaCollection convenience class to simplify iterating over collections from within JNI code Follow-up to https://codereview.webrtc.org/1788583004/. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by skvlad@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819553002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819553002/100001
Message was sent while issue was closed.
Description was changed from ========== Added the JNI interface to get and set RtpParameters and the maximum bitrate limits. Defined a JavaCollection convenience class to simplify iterating over collections from within JNI code Follow-up to https://codereview.webrtc.org/1788583004/. BUG= ========== to ========== Added the JNI interface to get and set RtpParameters and the maximum bitrate limits. Defined a JavaCollection convenience class to simplify iterating over collections from within JNI code Follow-up to https://codereview.webrtc.org/1788583004/. BUG= ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Added the JNI interface to get and set RtpParameters and the maximum bitrate limits. Defined a JavaCollection convenience class to simplify iterating over collections from within JNI code Follow-up to https://codereview.webrtc.org/1788583004/. BUG= ========== to ========== Added the JNI interface to get and set RtpParameters and the maximum bitrate limits. Defined a JavaCollection convenience class to simplify iterating over collections from within JNI code Follow-up to https://codereview.webrtc.org/1788583004/. BUG= Committed: https://crrev.com/303b3c21a4adf67359734b1bfbf5bdb9b25ad6c7 Cr-Commit-Position: refs/heads/master@{#12125} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/303b3c21a4adf67359734b1bfbf5bdb9b25ad6c7 Cr-Commit-Position: refs/heads/master@{#12125} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
