[jitsi-dev] [libjitsi] Adds a DebugTransformEngine. (#64)


#1

I had this lying around for a while and I thought that it might be helpful for somebody else. It requires some change in the PacketLoggingServiceImplementation as well so that it actually logs ARBITRARY packets.
You can view, comment on, or merge this pull request online at:

  https://github.com/jitsi/libjitsi/pull/64

-- Commit Summary --

  * Adds a DebugTransformEngine.

-- File Changes --

    M src/org/jitsi/impl/neomedia/MediaStreamImpl.java (21)
    A src/org/jitsi/impl/neomedia/transform/DebugTransformEngine.java (305)
    M src/org/jitsi/service/packetlogging/PacketLoggingService.java (7)

-- Patch Links --

https://github.com/jitsi/libjitsi/pull/64.patch
https://github.com/jitsi/libjitsi/pull/64.diff

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/64


#2

This does look useful.
Given how often this code is executed, I think it would make sense to move the enabled check to where the transformer is added to chain. To keep the property "confined" withn the DebugTransformer, you could use a static factory method that only returns an instance if enabled or simply expose the enabled through a property.

Please remove commented debug code and what is that // region stuff about?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/64#issuecomment-147615885


#3

Thanks very much for your valuable input @ibauersachs. I'll update the PR with an improved engine based on your comments. About the "//region .." comments, it's an IntelliJ IDEA feature that allows the [folding of custom regions with line comments](https://www.jetbrains.com/idea/help/folding-custom-regions-with-line-comments.html). It's can be handy if you're using that IDE, but I can remove it if you have a strong preference against it.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/64#issuecomment-148750872


#4

This PR is complemented by https://github.com/jitsi/jitsi/pull/171

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/64#issuecomment-148765127


#5

C# has them as a language feature and there have been endless debates of whether to use them or not. Being an Eclipse user I guess you can guess my preference. Please check with your teammates at Atlassian.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/64#issuecomment-148823989


#6

Thank you very much for sharing your thoughts on this Ingo. I have been using the controversial regions back when I was coding in C#, but I agree with you that they need to be removed from our code as it is non-standard practice.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/64#issuecomment-148831668


#7

Merged #64.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/64#event-439614860