Closed Bug 1086693 Opened 11 years ago Closed 10 years ago

Use autogenerated JNI bindings for SDK classes

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox34- fixed, firefox35+ fixed, firefox36+ fixed, fennec35+)

RESOLVED FIXED
Firefox 36
Tracking Status
firefox34 - fixed
firefox35 + fixed
firefox36 + fixed
fennec 35+ ---

People

(Reporter: snorp, Assigned: snorp)

References

Details

Attachments

(9 files, 9 obsolete files)

12.40 KB, patch
ckitching
: review+
Details | Diff | Splinter Review
11.44 KB, patch
ckitching
: review+
Details | Diff | Splinter Review
78.84 KB, patch
ckitching
: review+
Details | Diff | Splinter Review
23.52 KB, patch
snorp
: review+
Details | Diff | Splinter Review
2.10 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
120.46 KB, patch
blassey
: review+
Details | Diff | Splinter Review
5.44 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
5.04 KB, patch
Details | Diff | Splinter Review
2.05 KB, patch
Details | Diff | Splinter Review
Bug 1014614 checked in some code that was based on autogenerated JNI code, but had to be changed in order to make usable. We should actually generate that code instead.
Assignee: nobody → snorp
Attachment #8508824 - Flags: review?(nchen)
Attachment #8508824 - Flags: review?(chriskitching)
Attachment #8508824 - Flags: review?(blassey.bugs)
This patch actually checks in pre-generated output because I can't figure out how to export a generated header. :gps, can you help? widget/android/bindings/Makefile.in has the rules for generating stuff
Flags: needinfo?(gps)
Quick comment: It looks like you copypasta'd a ton of code from the annotation processor to make SDKGenerator. Wouldn't it make sense to share more of that? After all, all you *really* need in this patch is a way to provide input to the generation step of the annotation processor that doesn't rely on annotations. ... I'll give you a proper review when I'm not on a train. (I seem to be on trains a lot recently)
(In reply to Chris Kitching [:ckitching] from comment #3) > Quick comment: It looks like you copypasta'd a ton of code from the > annotation processor to make SDKGenerator. > > Wouldn't it make sense to share more of that? > > After all, all you *really* need in this patch is a way to provide input to > the generation step of the annotation processor that doesn't rely on > annotations. > > ... I'll give you a proper review when I'm not on a train. (I seem to be on > trains a lot recently) Yeah, quite a bit of that can/should be factored out. I can do that too, I guess, just trying to get stuff working :)
Comment on attachment 8508824 [details] [diff] [review] Use generated JNI bindings for MediaCodec and SurfaceTexture Review of attachment 8508824 [details] [diff] [review]: ----------------------------------------------------------------- This should not be checked in as-is because it doesn't meet build system standards. But getting there isn't too much effort. To handle generated files, you should define GENERATED_FILES in a moz.build. For each file in that list, make will evaluate the rule for that file during the "export" target. See media/libjpeg and jpeg_nbits_table.h for an example. Eventually we'll have something better where we can declare the commands used to perform generation, all from moz.build. See bug 883954. Ideally you could also define generated files in EXPORTS to have them copied to the objdir automatically. But I'm pretty sure that will fail. So, you need an INSTALL_TARGETS rule in your Makefile to install things to $(DIST)/include. Please request review from a build peer when these changes are made.
Flags: needinfo?(gps)
I can help with the generated files bits. ckitching: can you sketch a roadmap for landing this relative to the other changes you have in the works?
(In reply to Nick Alexander :nalexander from comment #6) > ckitching: can you sketch a > roadmap for landing this relative to the other changes you have in the works? The Java changes here will need to be altered to compensate for Bug 1067217. I have a mild preference for landing this after Bug 1067217, simply because I don't especially want to rebase the ridiculous patch in that bug. I really should automate my hg rebasing more. If there is no urgency on landing this one, I suggest simply assigning it to me and moving on to other things. I'll make the necessary changes to make it play nice with the other stuff that's in flight. On another note why not just stick the new classes in the existing GeneratedJNIWrappers.cpp? That way, you don't need to bother writing your own output routines, and can simplify this even further. An advantage of doing what you've done is that it doesn't experience the cyclic dependency that led to the other code being in the VCS - the library is invariant, so you can generate this stuff whenever you like, really. This leads us nicely to the more general problem that gps pointed out. Our current (pre-Bug 1067217) JNI wrapper generation pipeline looks like this: 1) Compile the generated C++ code. 2) Compile the generator program. 3) Compile the Java code. 4) Run the generator on the compiled Java code to generate the C++ code. 5) Compare the generated C++ code to the stuff we compiled earlier (that was found in the VCS). If they differ, fail and instruct the user to copy one to the other. Since we generate the C++ code in a later step than we want to use it, we're in trouble. The "correct" solution is to reorder the build process so the Java stuff happens before the C++ stuff. I gather from Nick that this is nontrivial and generally not worth the hassle. So, we stick the generated C++ in the VCS and error out when it changes. After Bug 1067217, the situation looks like this: 1) Compile the generated C++ code. 2) Compile the Java code with an annotation processor that generates the C++ code shortly after the parsing step 3) Compare the generated C++ code as before. The difference is subtle, but important. We no longer require the result of compiling the Java code to generate the C++ code. We're now running an actual, proper, Java annotation processor (See: http://docs.oracle.com/javase/7/docs/technotes/tools/windows/javac.html#processing ). This is invoked by javac during compilation (shortly after parsing) and handed chunks of the AST which have the desired annotations. This runs faster than scanning the bytecode for annotations, as well as being prettier, but the real reason we care (in this context, anyway) is: * Javac can be instructed to abort immediately following annotation processing. * Annotation processing can complete provided it parses. It need not type check, nor be capable of actually producing valid bytecode. This avoids some of the potential dependency problems associated with radically-reordering the build process. As a result, we could then do: 1) Run annotation processing on Fennec's Java source. (Generating our C++ code) 2) Compile the C++ we just generated (and whatever else) 3) Compile Fennec's Java code with the code-generating annotation processor turned off. 4) Party, because GPS no longer wants to throw us out the window. And make the madness go away. Is the second step worth it? Maybe not, but at least it stops being impossible now.
Trying to sort through ckitching's comment and discussions in #mobile, I conclude: * this is not an AP at all; it's a code generation step that we want to add to widget/android; * this is entangled with the existing AP code, but that's just because it's convenient. ckitching and I discussed this and he's happy to get the build system integration of this up to par, land it, and then continue his AP work on top of these changes. I'm going to dig in to the build ordering problems snorp was hitting now.
Comment on attachment 8508824 [details] [diff] [review] Use generated JNI bindings for MediaCodec and SurfaceTexture Review of attachment 8508824 [details] [diff] [review]: ----------------------------------------------------------------- Clearing the review until Kitching's concerns are resolved
Attachment #8508824 - Flags: review?(blassey.bugs)
This is one part of snorp's monolithic patch. I think ckitching is okay with this, but might as well have his stamp.
Attachment #8512320 - Flags: review?(chriskitching)
Comment on attachment 8512320 [details] [diff] [review] Part 1: Add SDKProcessor. r=ckitching Review of attachment 8512320 [details] [diff] [review]: ----------------------------------------------------------------- Very nice. We can kill off most of this slightly annoying code generation later (as mentioned on IRC)
Attachment #8512320 - Flags: review?(chriskitching) → review+
The generated sources and headers are built during the export tier. Since export is fully parallel, a new recurse dependency is needed to make build/annotationProcessors visible to widget/android/bindings. This is part 2 of 3 of snorp's original patch.
Attachment #8512325 - Flags: review?(gps)
This is part 3 of snorp's original patch. I'll let snorp flag blassey for review when he's tested and is ready.
Attachment #8508824 - Attachment is obsolete: true
Attachment #8508824 - Flags: review?(nchen)
Attachment #8508824 - Flags: review?(chriskitching)
Comment on attachment 8512325 [details] [diff] [review] Part 2: Generate and build Android SDK JNI wrappers. r=gps Review of attachment 8512325 [details] [diff] [review]: ----------------------------------------------------------------- Just nits. Nothing that I suspect you can't handle without rereview. ::: config/recurse.mk @@ +140,5 @@ > ifeq (.,$(DEPTH)) > # Interdependencies for parallel export. > js/xpconnect/src/export: dom/bindings/export xpcom/xpidl/export > accessible/xpcom/export: xpcom/xpidl/export > +widget/android/bindings/export: build/annotationProcessors/export Please document this. (Yes, the existing ones should be documented too.) ::: widget/android/bindings/Makefile.in @@ +1,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +ANNOTATION_PROCESSOR_JAR_FILES := $(DEPTH)/build/annotationProcessors/annotationProcessors.jar bonus points if you rename the Makefile.in-local variables to lowercase. I try to enforce the convention that UPPERCASE variables are special and have rules in rules.mk, etc. But I haven't been having much luck with that. Ignore this request if you are lazy. @@ +6,5 @@ > + > +MediaCodec.cpp: $(ANDROID_SDK)/android.jar mediacodec-classes.txt > + $(JAVA) -classpath $(ANNOTATION_PROCESSOR_JAR_FILES) org.mozilla.gecko.annotationProcessors.SDKProcessor $(ANDROID_SDK)/android.jar $(srcdir)/mediacodec-classes.txt $(CURDIR) MediaCodec > + > +MediaCodec.h: MediaCodec.cpp ; Kill the semicolon. @@ +11,5 @@ > + > +SurfaceTexture.cpp: $(ANDROID_SDK)/android.jar surfacetexture-classes.txt > + $(JAVA) -classpath $(ANNOTATION_PROCESSOR_JAR_FILES) org.mozilla.gecko.annotationProcessors.SDKProcessor $(ANDROID_SDK)/android.jar $(srcdir)/surfacetexture-classes.txt $(CURDIR) SurfaceTexture > + > +SurfaceTexture.h: SurfaceTexture.cpp ; Kill semicolon. @@ +20,5 @@ > + SurfaceTexture.h \ > + $(NULL) > + > +INSTALL_TARGETS += bindings_exports > +bindings_exports_FILES := $(bindings_exports) You could just make the assignment inline. The extra variable adds nothing except confusion.
Attachment #8512325 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #14) > Comment on attachment 8512325 [details] [diff] [review] > Part 2: Generate and build Android SDK JNI wrappers. r=gps > > Review of attachment 8512325 [details] [diff] [review]: > ----------------------------------------------------------------- > > Just nits. Nothing that I suspect you can't handle without rereview. > > ::: config/recurse.mk > @@ +140,5 @@ > > ifeq (.,$(DEPTH)) > > # Interdependencies for parallel export. > > js/xpconnect/src/export: dom/bindings/export xpcom/xpidl/export > > accessible/xpcom/export: xpcom/xpidl/export > > +widget/android/bindings/export: build/annotationProcessors/export > > Please document this. (Yes, the existing ones should be documented too.) Is a comment in this file explaining why the dependency exists sufficient, such as: # build/annotationProcessors produces a code generating JAR during its export tier that is used by widget/android/bindings during its export tier. > ::: widget/android/bindings/Makefile.in > @@ +1,5 @@ > > +# This Source Code Form is subject to the terms of the Mozilla Public > > +# License, v. 2.0. If a copy of the MPL was not distributed with this > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > + > > +ANNOTATION_PROCESSOR_JAR_FILES := $(DEPTH)/build/annotationProcessors/annotationProcessors.jar > > bonus points if you rename the Makefile.in-local variables to lowercase. I > try to enforce the convention that UPPERCASE variables are special and have > rules in rules.mk, etc. But I haven't been having much luck with that. > Ignore this request if you are lazy. I like this convention and will follow it. > @@ +6,5 @@ > > + > > +MediaCodec.cpp: $(ANDROID_SDK)/android.jar mediacodec-classes.txt > > + $(JAVA) -classpath $(ANNOTATION_PROCESSOR_JAR_FILES) org.mozilla.gecko.annotationProcessors.SDKProcessor $(ANDROID_SDK)/android.jar $(srcdir)/mediacodec-classes.txt $(CURDIR) MediaCodec > > + > > +MediaCodec.h: MediaCodec.cpp ; > > Kill the semicolon. Damn memory. The semi is necessary for empty dependency lists. > @@ +11,5 @@ > > + > > +SurfaceTexture.cpp: $(ANDROID_SDK)/android.jar surfacetexture-classes.txt > > + $(JAVA) -classpath $(ANNOTATION_PROCESSOR_JAR_FILES) org.mozilla.gecko.annotationProcessors.SDKProcessor $(ANDROID_SDK)/android.jar $(srcdir)/surfacetexture-classes.txt $(CURDIR) SurfaceTexture > > + > > +SurfaceTexture.h: SurfaceTexture.cpp ; > > Kill semicolon. > > @@ +20,5 @@ > > + SurfaceTexture.h \ > > + $(NULL) > > + > > +INSTALL_TARGETS += bindings_exports > > +bindings_exports_FILES := $(bindings_exports) > > You could just make the assignment inline. The extra variable adds nothing > except confusion. Yeah, copy-pasta.
This adds stuff to the SDK generator such that it avoids members that are above a specified API level.
Attachment #8519132 - Flags: review?(nalexander)
Attachment #8519132 - Flags: review?(chriskitching)
This allows us to initialize methods (and fields, etc) only when they are called, avoiding problems with things that are only available at certain API versions.
Attachment #8519135 - Flags: review?(nalexander)
Attachment #8519135 - Flags: review?(chriskitching)
This is a new exception handling mode that allows the caller to easily check if something went wrong. It could be enhanced in the future to return the exception instance or something, but this is good enough for now.
Attachment #8519136 - Flags: review?(nalexander)
Attachment #8519136 - Flags: review?(chriskitching)
Attachment #8512326 - Attachment is obsolete: true
Attachment #8519137 - Flags: review?(blassey.bugs)
Comment on attachment 8519132 [details] [diff] [review] Part 3: Don't generate members that are above a given API version Review of attachment 8519132 [details] [diff] [review]: ----------------------------------------------------------------- Seems mostly sane, though relies on some apparently magical Android Lint API I didn't know existed. Provided you've tested it and it does what it's supposed to I'm a fan. Most of my comments relate to code cleanup (in particular your filtering function seems like it can be simplified a fair bit). ::: build/annotationProcessors/SDKProcessor.java @@ +4,5 @@ > > package org.mozilla.gecko.annotationProcessors; > > +import com.android.tools.lint.checks.ApiLookup; > +import com.android.tools.lint.LintCliClient; I find my inability to find Javadocs for these classes quite disturbing. ... Presumably it all works... @@ +68,5 @@ > + System.out.println("MediaCodec version: " + sApiLookup.getClassVersion("android/media/MediaCodec")); > + System.out.println("MediaCodec.getInputBuffer version: " + > + sApiLookup.getCallVersion("android/media/MediaCodec", > + "getInputBuffer", > + "(I)Ljava/nio/ByteBuffer;")); Isn't this debugging code? @@ +147,5 @@ > long e = System.currentTimeMillis(); > System.out.println("SDK processing complete in " + (e - s) + "ms"); > } > > + private static Member[] sortAndFilterMembers(Member[] members) { It seems like you're only calling this from the bit a little later on where you do: generator.generateMembers(sortAndFilterMembers(clazz.getDeclaredConstructors())); ... etc. This means that each argument will be either a Constructor[], a Method[], or a Field[]. You should therefore probably refactor this to avoid the evil runtime type checks you've had to put into it a little later on. You might well end up with two specialised (and about three-line) filter functions and a Member[]-accepting sort function (prettymuch the one you had before). @@ +155,5 @@ > return a.getName().compareTo(b.getName()); > } > }); > > + ArrayList<Member> list = new ArrayList<Member>(); Use diamonds! ArrayList<Member> list = new ArrayList<>(); Type inference for the win. @@ +181,5 @@ > + } > + > + Member[] result = new Member[list.size()]; > + list.toArray(result); > + return result; It might be clearer to squish all three of these lines into one: return list.toArray(new Member[list.size()]); (I have a general distaste of relying on toArray's side effect) ::: build/annotationProcessors/utils/Utils.java @@ +319,5 @@ > } > > + public static String getTypeSignatureStringForClass(Class<?> clazz) { > + return clazz.getCanonicalName().replace('.', '/'); > + } It seems like this is what I really wanted to do when I wrote this: http://mxr.mozilla.org/mozilla-central/source/build/annotationProcessors/utils/Utils.java#592 You should probably do the obvious refactor of getStartupLineForClass to use your new utility function. Likewise: http://mxr.mozilla.org/mozilla-central/source/build/annotationProcessors/utils/Utils.java#344
Attachment #8519132 - Flags: review?(chriskitching) → review+
Comment on attachment 8519135 [details] [diff] [review] Part 4: Add a lazy initialization mode to CodeGenerator, and use it for SDK bindings Review of attachment 8519135 [details] [diff] [review]: ----------------------------------------------------------------- It'd be much easier to review this were it not for the fact that the vast majority of the patch seems to be simply renaming jEnv to env. ... Really? I'm a massive fan of ridiculously huge and aggressive refactoring patches (and routinely throw >300kb ones at rnewman), but please separate the refactors from the stuff that I have to actually turn my brain on to read. "Spot the real changes amongst the huge amount of variable renamings in Splinter" is not a fun game to play. I'm not sure that renaming this variable is a useful thing to bother with, but if you really want to then go for it... just don't make reviewing the interesting stuff harder by munging it all together. ::: build/annotationProcessors/CodeGenerator.java @@ +46,5 @@ > public CodeGenerator(Class<?> aClass, String aGeneratedName) { > + this(aClass, aGeneratedName, false); > + } > + > + public CodeGenerator(Class<?> aClass, String aGeneratedName, boolean aLazyInit) { The "aSomethingSomething" naming convention for parameters has fallen out of favour at Mozilla since the original code here was written. If you fancy another aggressive refactoring patch, feel free to go through absolutely all the generator code exterminating it. ::: build/annotationProcessors/SDKProcessor.java @@ +190,5 @@ > StringBuilder implementationFile, > StringBuilder headerFile) { > String generatedName = clazz.getSimpleName(); > > + CodeGenerator generator = new CodeGenerator(clazz, generatedName, true); Don't you want finer-grained control of what gets lazily-initialised? The initialisation is sort of expensive, so doing this at the granularity of the CodeGenerator seems a little sledgehammer-esque. More generally, might this program benefit from a config file where you can specify options for generated classes? There's a bunch of little things we might want to set slightly differently (the exception handling, optional parameters, etc.), and inventing a new kludge every time we want a new feature doesn't seem like it'll scale well. Some sort of config file format where you can specify classes to generate, members to exclude (or include), and options for each... Perhaps? ::: widget/android/AndroidBridge.cpp @@ +158,5 @@ > sBridge = bridge; > } > > bool > +AndroidBridge::Init(JNIEnv *env) Yay, noise!
Attachment #8519135 - Flags: review?(chriskitching)
Comment on attachment 8519136 [details] [diff] [review] Part 5: Add a 'catchException' mode to JNI generator Review of attachment 8519136 [details] [diff] [review]: ----------------------------------------------------------------- Seems a vaguely useful feature. Mostly whinging about code-cleanup-related things again. ::: build/annotationProcessors/CodeGenerator.java @@ +124,5 @@ > writeMethodBody(implementationSignature, theMethod, mClassToWrap, > aMethodTuple.mAnnotationInfo.isMultithreaded, > aMethodTuple.mAnnotationInfo.noThrow, > + aMethodTuple.mAnnotationInfo.narrowChars, > + aMethodTuple.mAnnotationInfo.catchException); This is starting to get silly. Perhaps we should refactor this to just take the AnnotationInfo object and have done with it. Don't bother for now, though: refactoring this as an annotation processor makes much of this madness go away. @@ +460,5 @@ > + .append(" *aResult = NS_ERROR_FAILURE;\n") > + .append(" }\n") > + .append(" } else if (aResult) {\n") > + .append(" *aResult = NS_OK;\n") > + .append(" }\n\n"); Javac can't constant fold across these calls. Your performance will be somewhat better if you refactor this as a single append call: .append(" if (env->ExceptionCheck()) {\n" + " env->ExceptionClear();\n" + ... etc... That way, javac can constant fold your use of + into one big string and you get one append call at runtime (what you have right now will actually compile to the eight calls you actually *wrote*, even though that doesn't make any kind of sense) @@ +557,5 @@ > + .append(" } else if (aResult) {\n") > + .append(" *aResult = NS_OK;\n") > + .append(" }\n\n"); > + } > + Such magnificent copypasta! You might want to not duplicate this code.... ::: build/annotationProcessors/utils/GeneratableElementIterator.java @@ +101,5 @@ > + // Determine if we should catch exceptions > + final Method catchExceptionMethod = annotationType.getDeclaredMethod("catchException"); > + catchExceptionMethod.setAccessible(true); > + catchException = (Boolean) catchExceptionMethod.invoke(annotation); > + *twitch* I hate this API so very, very much.
Attachment #8519136 - Flags: review?(chriskitching) → review+
(In reply to Chris Kitching [:ckitching] from comment #21) > Comment on attachment 8519135 [details] [diff] [review] > Part 4: Add a lazy initialization mode to CodeGenerator, and use it for SDK > bindings > > Review of attachment 8519135 [details] [diff] [review]: > ----------------------------------------------------------------- > > It'd be much easier to review this were it not for the fact that the vast > majority of the patch seems to be simply renaming jEnv to env. > > ... Really? > > I'm a massive fan of ridiculously huge and aggressive refactoring patches > (and routinely throw >300kb ones at rnewman), but please separate the > refactors from the stuff that I have to actually turn my brain on to read. > "Spot the real changes amongst the huge amount of variable renamings in > Splinter" is not a fun game to play. > > I'm not sure that renaming this variable is a useful thing to bother with, > but if you really want to then go for it... just don't make reviewing the > interesting stuff harder by munging it all together. Do you want to lay off the hostility just a little bit? This kind of thing is completely uncalled for. I didn't change the variable name just for the hell of it. The generator code uses the macros in AndroidBridgeUtilties, which (until this patch), assumes 'jEnv' as the JNIEnv*. We were using 'env' in the generated method bodies, so something needed changing. We use 'env' almost everywhere else, so I changed to that. I realize now we could easily skip macros altogether in the generator. That would be a much smaller patch, so I'll make that change. > > ::: build/annotationProcessors/CodeGenerator.java > @@ +46,5 @@ > > public CodeGenerator(Class<?> aClass, String aGeneratedName) { > > + this(aClass, aGeneratedName, false); > > + } > > + > > + public CodeGenerator(Class<?> aClass, String aGeneratedName, boolean aLazyInit) { > > The "aSomethingSomething" naming convention for parameters has fallen out of > favour at Mozilla since the original code here was written. If you fancy > another aggressive refactoring patch, feel free to go through absolutely > all the generator code exterminating it. > > ::: build/annotationProcessors/SDKProcessor.java > @@ +190,5 @@ > > StringBuilder implementationFile, > > StringBuilder headerFile) { > > String generatedName = clazz.getSimpleName(); > > > > + CodeGenerator generator = new CodeGenerator(clazz, generatedName, true); > > Don't you want finer-grained control of what gets lazily-initialised? > The initialisation is sort of expensive, so doing this at the granularity of > the CodeGenerator seems a little sledgehammer-esque. Maybe, but this is 1.0. No need to get fancy right off the bat. The current approach works fine for what I need. > > More generally, might this program benefit from a config file where you can > specify options for generated classes? There's a bunch of little things we > might want to set slightly differently (the exception handling, optional > parameters, etc.), and inventing a new kludge every time we want a new > feature doesn't seem like it'll scale well. > > Some sort of config file format where you can specify classes to generate, > members to exclude (or include), and options for each... Perhaps? Maybe, but again, this is 1.0. I don't see the options really changing much as far as binding SDK classes. They should all be callable from all threads, they all can throw exceptions, etc. I guess maybe you could want wide strings. > > ::: widget/android/AndroidBridge.cpp > @@ +158,5 @@ > > sBridge = bridge; > > } > > > > bool > > +AndroidBridge::Init(JNIEnv *env) > > Yay, noise!
(In reply to Chris Kitching [:ckitching] from comment #20) > ::: build/annotationProcessors/SDKProcessor.java > @@ +4,5 @@ > > > > package org.mozilla.gecko.annotationProcessors; > > > > +import com.android.tools.lint.checks.ApiLookup; > > +import com.android.tools.lint.LintCliClient; > > I find my inability to find Javadocs for these classes quite disturbing. > > ... Presumably it all works... Yeah, it's definitely a little shady. It's part of the SDK tools, and could presumably change at any time. Hopefully we won't have too many problems. > > @@ +68,5 @@ > > + System.out.println("MediaCodec version: " + sApiLookup.getClassVersion("android/media/MediaCodec")); > > + System.out.println("MediaCodec.getInputBuffer version: " + > > + sApiLookup.getCallVersion("android/media/MediaCodec", > > + "getInputBuffer", > > + "(I)Ljava/nio/ByteBuffer;")); > > Isn't this debugging code? Oops, yes it is. > > @@ +147,5 @@ > > long e = System.currentTimeMillis(); > > System.out.println("SDK processing complete in " + (e - s) + "ms"); > > } > > > > + private static Member[] sortAndFilterMembers(Member[] members) { > > It seems like you're only calling this from the bit a little later on where > you do: > generator.generateMembers(sortAndFilterMembers(clazz. > getDeclaredConstructors())); > ... etc. > > > This means that each argument will be either a Constructor[], a Method[], or > a Field[]. > You should therefore probably refactor this to avoid the evil runtime type > checks you've had to put into it a little later on. > > You might well end up with two specialised (and about three-line) filter > functions and a Member[]-accepting sort function (prettymuch the one you had > before). Yup, probably good. I'll change it. > > @@ +155,5 @@ > > return a.getName().compareTo(b.getName()); > > } > > }); > > > > + ArrayList<Member> list = new ArrayList<Member>(); > > Use diamonds! > > ArrayList<Member> list = new ArrayList<>(); > > Type inference for the win. > > @@ +181,5 @@ > > + } > > + > > + Member[] result = new Member[list.size()]; > > + list.toArray(result); > > + return result; > > It might be clearer to squish all three of these lines into one: > > return list.toArray(new Member[list.size()]); > > (I have a general distaste of relying on toArray's side effect) Ah, I didn't even realize it also returned the Array. Yes, that's much nicer. > > ::: build/annotationProcessors/utils/Utils.java > @@ +319,5 @@ > > } > > > > + public static String getTypeSignatureStringForClass(Class<?> clazz) { > > + return clazz.getCanonicalName().replace('.', '/'); > > + } > > It seems like this is what I really wanted to do when I wrote this: > > http://mxr.mozilla.org/mozilla-central/source/build/annotationProcessors/ > utils/Utils.java#592 > > You should probably do the obvious refactor of getStartupLineForClass to use > your new utility function. > > Likewise: > http://mxr.mozilla.org/mozilla-central/source/build/annotationProcessors/ > utils/Utils.java#344 Yup
(In reply to Chris Kitching [:ckitching] from comment #22) > Comment on attachment 8519136 [details] [diff] [review] > Part 5: Add a 'catchException' mode to JNI generator > > Review of attachment 8519136 [details] [diff] [review]: > ----------------------------------------------------------------- > > Seems a vaguely useful feature. Mostly whinging about code-cleanup-related > things again. > > ::: build/annotationProcessors/CodeGenerator.java > @@ +124,5 @@ > > writeMethodBody(implementationSignature, theMethod, mClassToWrap, > > aMethodTuple.mAnnotationInfo.isMultithreaded, > > aMethodTuple.mAnnotationInfo.noThrow, > > + aMethodTuple.mAnnotationInfo.narrowChars, > > + aMethodTuple.mAnnotationInfo.catchException); > > This is starting to get silly. Perhaps we should refactor this to just take > the AnnotationInfo object and have done with it. > > Don't bother for now, though: refactoring this as an annotation processor > makes much of this madness go away. Yeah, I agree it is pretty silly. Don't you already have a patch that fixes it? > > @@ +460,5 @@ > > + .append(" *aResult = NS_ERROR_FAILURE;\n") > > + .append(" }\n") > > + .append(" } else if (aResult) {\n") > > + .append(" *aResult = NS_OK;\n") > > + .append(" }\n\n"); > > Javac can't constant fold across these calls. > > Your performance will be somewhat better if you refactor this as a single > append call: > > .append(" if (env->ExceptionCheck()) {\n" + > " env->ExceptionClear();\n" + > ... etc... > > > That way, javac can constant fold your use of + into one big string and you > get one append call at runtime (what you have right now will actually > compile to the eight calls you actually *wrote*, even though that doesn't > make any kind of sense) Not sure we care about that too much here, but sure, why not. > > @@ +557,5 @@ > > + .append(" } else if (aResult) {\n") > > + .append(" *aResult = NS_OK;\n") > > + .append(" }\n\n"); > > + } > > + > > Such magnificent copypasta! > > You might want to not duplicate this code.... Indeed, I thought I had fixed that earlier, but I guess not. > > ::: build/annotationProcessors/utils/GeneratableElementIterator.java > @@ +101,5 @@ > > + // Determine if we should catch exceptions > > + final Method catchExceptionMethod = annotationType.getDeclaredMethod("catchException"); > > + catchExceptionMethod.setAccessible(true); > > + catchException = (Boolean) catchExceptionMethod.invoke(annotation); > > + > > *twitch* > > I hate this API so very, very much.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #23) > Do you want to lay off the hostility just a little bit? This kind of thing > is completely uncalled for. That really wasn't intended to come across as hostile. Sorry about that. Apparently I shouldn't try humour in the middle of the night in code reviews (or, quite possibly, ever.). (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #25) > Yeah, I agree it is pretty silly. Don't you already have a patch that fixes > it? Yes, I think I sorted this out in one of the other bugs. Mostly just passing comment on my lack of forethought, as various people have bolted new options on since the first implementation and the pattern I started hasn't scaled prettily. > I didn't change the variable name just for the hell of it. The generator > code uses the macros in AndroidBridgeUtilties, which (until this patch), > assumes 'jEnv' as the JNIEnv*. We were using 'env' in the generated method > bodies, so something needed changing. We use 'env' almost everywhere else, > so I changed to that. I realize now we could easily skip macros altogether > in the generator. That would be a much smaller patch, so I'll make that > change. That makes a lot more sense. Supporting the macros might be a good idea, mostly a matter of style. Do what you like best. > Maybe, but again, this is 1.0. I don't see the options really changing much > as far as binding SDK classes. They should all be callable from all threads, > they all can throw exceptions, etc. I guess maybe you could want wide > strings. True. I guess it's not impossible someone might want to noThrow-ify a library function, but that'd probably not be a good idea anyway.
Comment on attachment 8519137 [details] [diff] [review] Part 6: Use generated bindings for MediaCodec and SurfaceTexture Review of attachment 8519137 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/fmp4/android/AndroidDecoderModule.cpp @@ +473,5 @@ > + nsresult res; > + mInputBuffers = (jobjectArray) env->NewGlobalRef(mDecoder->GetInputBuffers(&res)); > + if (NS_FAILED(res)) { > + return res; > + } don't you need to return NS_OK here? @@ +488,5 @@ > + nsresult res; > + mOutputBuffers = (jobjectArray) env->NewGlobalRef(mDecoder->GetOutputBuffers(&res)); > + if (NS_FAILED(res)) { > + return res; > + } don't you need to return NS_OK here?
As discussed on IRC, the runtime checks really have the smallest and most readable code, but I did do the other cleanups.
Attachment #8519132 - Attachment is obsolete: true
Attachment #8519132 - Flags: review?(nalexander)
Attachment #8520055 - Flags: review?(chriskitching)
This version abandons the use of the macros in the generator, which requires far fewer renamings (except in GeneratedJNIWrappers, obviously).
Attachment #8519135 - Attachment is obsolete: true
Attachment #8519135 - Flags: review?(nalexander)
Attachment #8520057 - Flags: review?(chriskitching)
Cleanups, carrying r+ from ckitching
Attachment #8519136 - Attachment is obsolete: true
Attachment #8519136 - Flags: review?(nalexander)
Attachment #8520058 - Flags: review+
Attachment #8520055 - Flags: review?(chriskitching) → review+
Comment on attachment 8520057 [details] [diff] [review] Part 4: Add a lazy initialization mode to CodeGenerator, and use it for SDK bindings Review of attachment 8520057 [details] [diff] [review]: ----------------------------------------------------------------- To clarify: I wasn't opposed to your renaming, just suggesting it might've been nicer to have split that out into another patch. ... It was a stupid comment to have made. I'll leave it up to you to decide if it's a good idea for the generated code to use the macros: certainly, it gets pretty fugly in this patch when you make it stop using them. Have an r+. I'm also okay with you using the other renaming-heavy version if you want prettier generated code. Apologies for any confusion. ::: widget/android/GeneratedJNIWrappers.cpp @@ +2391,5 @@ > + mClipboardClass = AndroidBridge::GetClassGlobalRef(env, "org/mozilla/gecko/util/Clipboard"); > + jClearText = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "clearText", "()V"); > + jGetClipboardTextWrapper = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "getText", "()Ljava/lang/String;"); > + jHasText = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "hasText", "()Z"); > + jSetClipboardText = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "setText", "(Ljava/lang/CharSequence;)V"); Suddenly, I find myself missing the macros. :/
Attachment #8520057 - Flags: review?(chriskitching) → review+
(In reply to Chris Kitching [:ckitching] from comment #31) > Comment on attachment 8520057 [details] [diff] [review] > Part 4: Add a lazy initialization mode to CodeGenerator, and use it for SDK > bindings > > Review of attachment 8520057 [details] [diff] [review]: > ----------------------------------------------------------------- > > To clarify: I wasn't opposed to your renaming, just suggesting it might've > been nicer to have split that out into another patch. > > ... It was a stupid comment to have made. > > I'll leave it up to you to decide if it's a good idea for the generated code > to use the macros: certainly, it gets pretty fugly in this patch when you > make it stop using them. > > Have an r+. I'm also okay with you using the other renaming-heavy version if > you want prettier generated code. Apologies for any confusion. > > ::: widget/android/GeneratedJNIWrappers.cpp > @@ +2391,5 @@ > > + mClipboardClass = AndroidBridge::GetClassGlobalRef(env, "org/mozilla/gecko/util/Clipboard"); > > + jClearText = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "clearText", "()V"); > > + jGetClipboardTextWrapper = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "getText", "()Ljava/lang/String;"); > > + jHasText = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "hasText", "()Z"); > > + jSetClipboardText = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "setText", "(Ljava/lang/CharSequence;)V"); > > Suddenly, I find myself missing the macros. :/ Yeah, it's certainly a lot more verbose. I didn't like the renaming either, so I think I prefer this by a little. I guess we could also introduce new macros? Ugh.
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #27) > Comment on attachment 8519137 [details] [diff] [review] > Part 6: Use generated bindings for MediaCodec and SurfaceTexture > > Review of attachment 8519137 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/fmp4/android/AndroidDecoderModule.cpp > @@ +473,5 @@ > > + nsresult res; > > + mInputBuffers = (jobjectArray) env->NewGlobalRef(mDecoder->GetInputBuffers(&res)); > > + if (NS_FAILED(res)) { > > + return res; > > + } > > don't you need to return NS_OK here? > > @@ +488,5 @@ > > + nsresult res; > > + mOutputBuffers = (jobjectArray) env->NewGlobalRef(mDecoder->GetOutputBuffers(&res)); > > + if (NS_FAILED(res)) { > > + return res; > > + } > > don't you need to return NS_OK here? Indeed. I also wasn't even checking that return value. New patch coming.
Address review comments from Brad
Attachment #8519137 - Attachment is obsolete: true
Attachment #8519137 - Flags: review?(blassey.bugs)
Attachment #8520068 - Flags: review?(gpascutto)
Attachment #8520068 - Flags: review?(blassey.bugs)
Comment on attachment 8520068 [details] [diff] [review] Part 6: Use generated bindings for AndroidMediaCodec and AndroidSurfaceTexture Review of attachment 8520068 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/fmp4/android/AndroidDecoderModule.cpp @@ +352,5 @@ > if (sample) { > // We have a sample, try to feed it to the decoder > + int inputIndex = mDecoder->DequeueInputBuffer(DECODER_TIMEOUT, &res); > + if (NS_FAILED(res)) { > + printf_stderr("exiting decoder loop due to exception while dequeuing input\n"); No cleaner logging available here? ::: gfx/gl/AndroidSurfaceTexture.cpp @@ +105,5 @@ > aContext->fGenTextures(1, &mTexture); > > + nsresult res; > + mSurfaceTexture->AttachToGLContext(mTexture, &res); > + return NS_SUCCEEDED(res); Must this be a bool? If I were GCC I'd warn about "Possible loss of precision" here. @@ +182,3 @@ > if (mSurfaceTexture) { > + GeckoAppShell::UnregisterSurfaceTextureFrameListener(mSurfaceTexture->wrappedObject()); > + delete mSurfaceTexture; If you're deleting in the destructor, maybe AutoPtr/UniquePtr? @@ +185,5 @@ > mSurfaceTexture = nullptr; > } > > if (mSurface) { > + delete mSurface; Same. (But even more so as there's no unregister)
Attachment #8520068 - Flags: review?(gpascutto) → review+
(In reply to Gian-Carlo Pascutto [:gcp] from comment #35) > Comment on attachment 8520068 [details] [diff] [review] > Part 6: Use generated bindings for AndroidMediaCodec and > AndroidSurfaceTexture > > Review of attachment 8520068 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/fmp4/android/AndroidDecoderModule.cpp > @@ +352,5 @@ > > if (sample) { > > // We have a sample, try to feed it to the decoder > > + int inputIndex = mDecoder->DequeueInputBuffer(DECODER_TIMEOUT, &res); > > + if (NS_FAILED(res)) { > > + printf_stderr("exiting decoder loop due to exception while dequeuing input\n"); > > No cleaner logging available here? Yeah, I'm going to do something different there in a new patch. > > ::: gfx/gl/AndroidSurfaceTexture.cpp > @@ +105,5 @@ > > aContext->fGenTextures(1, &mTexture); > > > > + nsresult res; > > + mSurfaceTexture->AttachToGLContext(mTexture, &res); > > + return NS_SUCCEEDED(res); > > Must this be a bool? If I were GCC I'd warn about "Possible loss of > precision" here. There isn't really anything you can do to recover from this failing, so bool is the same as NS_OK/NS_ERROR_FAILURE. > > @@ +182,3 @@ > > if (mSurfaceTexture) { > > + GeckoAppShell::UnregisterSurfaceTextureFrameListener(mSurfaceTexture->wrappedObject()); > > + delete mSurfaceTexture; > > If you're deleting in the destructor, maybe AutoPtr/UniquePtr? Yup, good catch. I use that in other places, not sure why I didn't here. > > @@ +185,5 @@ > > mSurfaceTexture = nullptr; > > } > > > > if (mSurface) { > > + delete mSurface; > > Same. (But even more so as there's no unregister) Yup
Trivial patch that adds a Makefile target for updating GeneratedJNIWrappers.cpp/h and backs up whatever is there currently.
Attachment #8520165 - Flags: review?(nalexander)
I also fixed up some other potential badness where we weren't using a [ns]RefPtr for AndroidSurfaceTexture.
Attachment #8520068 - Attachment is obsolete: true
Attachment #8520068 - Flags: review?(blassey.bugs)
Attachment #8520166 - Flags: review?(gpascutto)
Comment on attachment 8520165 [details] [diff] [review] Add a Makefile target for updating GeneratedJNIWrappers Review of attachment 8520165 [details] [diff] [review]: ----------------------------------------------------------------- Sure, as you wish. Why silence the actual commands? Make your life easier by printing the long file paths to the console so you can copy and paste them.
Attachment #8520165 - Flags: review?(nalexander) → review+
tracking-fennec: --- → ?
[Tracking Requested - why for this release]: As I understand it, this is needed to fix a top-crash on beta
This addresses gcp reviews, and fixes up some RefPtr usage with AndroidSurfaceTexture
Attachment #8520166 - Attachment is obsolete: true
Attachment #8520166 - Flags: review?(gpascutto)
Attachment #8520739 - Flags: review?(blassey.bugs)
Attachment #8520739 - Flags: review?(blassey.bugs) → review+
Comment on attachment 8512320 [details] [diff] [review] Part 1: Add SDKProcessor. r=ckitching Approval Request Comment [Feature/regressing bug #]: bug 1014614 [User impact if declined]: crashes [Describe test coverage new/current, TBPL]: local only [Risks and why]: could potentially introduce more crashes, but only in the new MediaCodec backend. Expectation is that things will be better, however. [String/UUID change made/needed]: none
Attachment #8512320 - Flags: approval-mozilla-beta?
Attachment #8512320 - Flags: approval-mozilla-aurora?
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #42) > Comment on attachment 8512320 [details] [diff] [review] > Part 1: Add SDKProcessor. r=ckitching > > Approval Request Comment > [Feature/regressing bug #]: bug 1014614 > [User impact if declined]: crashes > [Describe test coverage new/current, TBPL]: local only > [Risks and why]: could potentially introduce more crashes, but only in the > new MediaCodec backend. Expectation is that things will be better, however. > [String/UUID change made/needed]: none Note that I want to land all of the patches on this bug, not just the one.
Backed out due to build failure. Lack of --enable-warnings-as-errors bites me again. cc1plus: error: MediaCodec.h: not a directory [-Werror] cc1plus: error: SurfaceTexture.h: not a directory [-Werror] cc1plus: all warnings being treated as errors Nick, any idea what's going on there? I'm looking into it, but thought you may know.
Flags: needinfo?(nalexander)
The problem comes from here: -IMediaCodec.h -ISurfaceTexture.h I don't know what part of Makefile.in is causing that
I replaced GENERATED_INCLUDES with GENERATED_FILES in widget/android/bindings/moz.build. This gets rid of the incorrect -I flags, but I'm not totally sure it's correct.
Attachment #8512325 - Attachment is obsolete: true
Flags: needinfo?(nalexander)
Attachment #8520913 - Flags: review?(gps)
Comment on attachment 8512320 [details] [diff] [review] Part 1: Add SDKProcessor. r=ckitching We're not going to take this fix on Beta and instead will disable the new media backend in bug 1097276.
Attachment #8512320 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8520913 [details] [diff] [review] Part 2: Generate and build Android SDK JNI wrappers. r=gps Review of attachment 8520913 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, the LOCAL_INCLUDES was my bad. This looks fine.
Attachment #8520913 - Flags: review?(gps) → review+
snorp: I would have pushed except these patches look stale. So instead I've uploaded updated version of Part 2 and Part 7 and you can push. If you can, add a .PHONY: update-generated-wrappers just below the definition in m/a/b/Makefile.in. (It's not a big deal, just good form; and I forgot.)
Flags: needinfo?(snorp)
tracking-fennec: ? → 35+
Flags: needinfo?(snorp)
(In reply to Nick Alexander :nalexander from comment #52) > snorp: I would have pushed except these patches look stale. So instead I've > uploaded updated version of Part 2 and Part 7 and you can push. If you can, > add a > > .PHONY: update-generated-wrappers > > just below the definition in m/a/b/Makefile.in. (It's not a big deal, just > good form; and I forgot.) Thanks. I added the .PHONY line.
Attachment #8512320 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1099345
Depends on: 1099501
Comment on attachment 8512320 [details] [diff] [review] Part 1: Add SDKProcessor. r=ckitching Alright, as discussed, we're going to try this on Beta once again. The patches are on m-c and Aurora and have solved (AFAIK) all of the crashes similar to bug 1091599.
Attachment #8512320 - Flags: approval-mozilla-beta- → approval-mozilla-beta?
Comment on attachment 8512320 [details] [diff] [review] Part 1: Add SDKProcessor. r=ckitching As snorp said, we're going to try this in beta10 again. If there is any significant fallout, the plan is to back this out of beta and prepare for an L specific point release. Beta+
Attachment #8512320 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8520055 [details] [diff] [review] Part 3: Don't generate members that are above a given API version [Triage Comment]
Attachment #8520055 - Flags: approval-mozilla-beta+
Attachment #8520058 - Flags: approval-mozilla-beta+
Attachment #8520057 - Flags: approval-mozilla-beta+
Attachment #8520165 - Flags: approval-mozilla-beta+
Attachment #8520739 - Flags: approval-mozilla-beta+
Attachment #8520913 - Flags: approval-mozilla-beta+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: