Closed GoogleCodeExporter closed 9 years ago
In looking into this, I found it very hard to follow the code in
RtpStreamReceiver.java and RtpStreamSender,java.
As I worked to find the problem, I made a series of changes to the code to tidy
it
up, and to make it clearer what is going on. In doing so, I discovered a
number of
other problems with this code.
The patch attached therefore fixes a number of problems:
- cosmetic code cleanups:
- fix indentation
- remove "TAB SPACE" sequences
- remove "SPACE EOL" sequences
- add some line breaks between code sections
- commentary
- added comments to explain what each code section does
- code simplifications that don't change app behavior:
- changed "ring", the buffer slot pointer, to be a simple index,
rather than a byte offset
- G711.[au]law2linear methods now take buffer offset
- calc() split into separate methods: silence() and audgain()
- calc1(), calc5(), calc10() removed; subsumed into audgain()
- code changes that do change app behavior:
- do comfort noise addition, even for speakerphone
- a/v sync delay no longer uses incorrect buffer size (in
ui/VideoCamera.java)
- alert tone placed in current buffer slot, not slot 0
- audio gain control function added for received audio
Since the patch is a little long, I am submitting it here for review prior to
committing it.
Original comment by jropal...@gmail.com
on 10 Feb 2010 at 6:52
Attachments:
Thanks for discussing this first.
Your code simplifications actually change app behavior. Code was splitted for
optimization purposes. The modified clipping checks won't work.
I propose that you undo them to focus on the intended code changes thus
avoiding to
introduce any bugs to a well tested code. Also please post a diff file ignoring
any
white space changes to make it easier to read. I would even leave the white
spaces
completely untouched to make the version control as transparent as possible.
This is
more important than the white spaces itself.
Back to the headline of this bug report: earpiece gain should be set as volume
in
restoreVolume(). This works with my phone.
Original comment by pmerl...@googlemail.com
on 10 Feb 2010 at 9:32
Nothing is committed yet; I have the changes in a separate copy here and won't
commit
until things are agreed.
Interesting that the audio gain is working on your phone. Not working on mine.
Nexus One here.
I looked at restoreVolume() and saw that it sets the gains of STREAM_RING and
STREAM_MUSIC but not the gain of STREAM_VOICE_CALL, so figured that code was for
something else. Perhaps STREAM_VOICE_CALL is also needed here? But I then
looked at
how the micgain was handled in RtpStreamSender and decided to duplicate that
solution
for the earpiece gain in RtpStreamReceiver. On my N1, the audio gain does now
work.
But if you think that adding STREAM_VOICE_CALL in restoreVolume might be a simpler
solution, I can try that. Or replacing STREAM_MUSIC with STREAM_VOICE_CALL?
As for calc(), calc1(), calc5() and calc10(), on examining them they all do the
same
thing using (almost) the same clipping value:
calc: 6550*5 32750
calc1: no clipping
calc5: 16350<<1 32700
calc10: 8150<<2 32600
I could not see why these different values were being used, and it did not make
sense: gain*5 clip at one value, gain*2 clip at a lower value, gain*4 clip at
an even
lower value. Why? So, in combining the functions, I made them all 32767. On
my
phone, this simpler code appears to work fine.
The other bugs fixed here also need review:
- outgoing comfort noise is only added when NOT in call
(speakermode == MODE_NORMAL) but not when actually in a call
- a/v sync uses fixed buffer multiple of 1024, even when frame_size is 160
- alert tone always placed in buffer slot 0 rather than current buffer
slot (works but this is bad because recorded audio might be xmitted later)
As for the whitespace fixing, I can do that later, no problem. I will do that
as a
separate commit and also add the comments as a separate commit too. No point in
losing that work.
I have no problem taking this large patch and splitting it into separate diffs,
each
ultimately done as a separate commit. They'd be:
- whitespaces and indentation (no change to code function)
- add more comments (no change to code function)
- fix comfort noise generation
- simplify/merge the four audio gain functions, if possible
- fix a/v delay ring buffer offset and alert tone buffer slot
- fix received audio gain
Original comment by jropal...@gmail.com
on 10 Feb 2010 at 11:19
- STREAM_MUSIC is used instead of STREAM_VOICE_CALL (see separate issue report
on
this subject).
- It's the check for clipping that does not work with your code (it's a short!).
- Noise generation is for speakerphone only.
The only "good part" of your changes is the change to VideoCamera at this time.
Please further investigate in the audio gain problem. It should work with
STREAM_MUSIC.
Original comment by pmerl...@googlemail.com
on 11 Feb 2010 at 4:20
OK, I've read issue 270 and also done a simple test, changing MUSIC to
VOICE_CALL
throughout (in the code from trunk, not in my patched code). I can report that
the
Earpiece Gain settings DO work with VOICE_CALL on the Nexus One which is a 2.1
device.
But I can also report that using the audgain() method in RtpStreamReceiver and
keeping MUSIC, as I did before, also solves the problem.
Anyway, I will let you work through the discussion in issue 270 first, rather
than
applying a separate patch here.
I note, while testing this, that the phone's volume switch changes the Ring
Volume,
even when in a call. It might make more sense to have the volume switch change
the
Media Volume when a call is in progress?
As for the other issues...
- I will fix my combining of calc/calc1/calc5/calc10 to use an int to test the
clipping. Thanks for catching that.
- I think silence detection/comfort noise generation should always be done
(speaker or handset). In fact, when on a speakerphone, there is less likely
to be total silence due to ambient room noise, but when on a handset silence
can lead to "hello are you still there?" I'd almost argue it is not needed
if on speakerphone.
- OK on the ring offset for the a/v sync.
I will work to separate out these issues into separate diffs and post them in
separate issue tickets for review prior to commiting. This will probably take
a few
days.
Original comment by jropal...@gmail.com
on 11 Feb 2010 at 10:53
When using STREAM_MUSIC in Sipdroid it should change media volume while in a
call.
Interesting that audio gain which uses track volume changes does not work in
this
case. Did you verify that?
Don't touch the calc routines please. An int would take more processing time.
They
have already undergone speed testing.
Noise is not generated for silence. This is used as an echo block for
speakerphone.
I have already taken over your change of the ring offset for the next release.
Original comment by pmerl...@googlemail.com
on 12 Feb 2010 at 9:22
If I use your latest .apk, the phone's volume switch does change the Media
Volume
when in a call. But if I change the code to use VOICE_DATA instead of MUSIC,
the
volume switch then changes the Ringer Volume when in a call.
I will abandon this whole patch for now.
But something is needed to make the handset/earpiece audio louder on the N1.
It is
very hard to hear, especially when outdoors, even with the Media Volume all the
way
up. By comparison, calls on the GSM phone and also playing of music is very
much
louder, and requires the media volume to be set to about 50% for a comfortable
level.
Adding the audgain() code to RtpStreamReceiver and then setting Earpiece Gain to
High or Highest in SIPdroid does fix this.
Do let me know if there's anything I can help with in testing alternative fixes
or in
applying this one.
Original comment by jropal...@gmail.com
on 13 Feb 2010 at 5:24
What I asked you to verify is if you are sure there is no change on the N1 for
different earpiece gains. This is not done by changing stream volume (as with
the
volume switches). It is done by changing the track volume. So if different
earpiece
gains don't result in different volumes heard in the earpiece there must be a
bug in
changing track volumes.
Original comment by pmerl...@googlemail.com
on 14 Feb 2010 at 9:33
Well, I believe it was not working at the time I started to look into this,
else I
would not have embarked on it.
However, I loaded 1.3.14 from the market today and changing the Earpiece
Setting does
indeed now change the volume in that version.
Not sure what code change fixed it, or if this was in fact due to other phone
stuff
that is no longer relevant.
Original comment by jropal...@gmail.com
on 16 Feb 2010 at 10:27
Original issue reported on code.google.com by
jropal...@gmail.com
on 10 Feb 2010 at 6:47