Re: Official Submission for SC-Avmailbox

Hi Ryan,

Ryan Ricard wrote:
>> -- I didn't find the tape.png and
default_outgoing_mesage.wav resouce
>> files so I've fetched them from your av-mailbox
repository. Let me know
>> if they are not the version that you intended to
submit.
> 
> I had them attached to the first message I sent, but
they weren't in
> the patch. But you've got the correct files so it's all
good  

> 

Oh ... I wonder where my head was when I wrote this. You are
right,
sorry I missed it.

>>
>> -- A suggestion. Wouldn't it be a better idea to
have file names stamped
>> in a more user friendly way? I was thinking about
including the name of
>> the first call participant, and possibly a human
readable date. We don't
>> seem to be having a user interface for the mailbox
right now so this
>> would definitely ease testing.
> 
> Oh yeah, I hadn't realized that the name of the call
participant was
> so readily available. How does
'Message.Name.MMDDYYYYHHMMSS.wav'
> sound?

How about 'Message.YYYYMMDDHHMMSS.Name.wav'? This way they'd
be order
chronologically when listing the contents of the directory.

>> * MediaService
>> -- Having the file name as the sole parameter when
setting a data sink
>> might be insufficient. Don't you think that we
should also content
>> descriptors explaining the formats that we'd like
to be recorded in this
>> file?
> 
> That makes sense. Is there a straightforward way in JMF
to test
> whether or not we can record in a particular format?

This is a lucky coincidence . The
latest patch from Sympho contained
just the methods we need. Have a look at

public String[] getSupportedAudioEncodings();
public String[] getSupportedVideoEncodings();

This makes me think however that we should probably add a
number of
static fields for the most popular encodings. Could you
please do this
if you have the time to? You could have a look at the media
control
class for the definitions (that are actually the standard
SDP ones).

>> -- I am wondering (and I'd really like to have your
opinion here)
>> whether it wouldn't be better to give more
granularity to the media
>> service, and kind of make it look like a _very_
simplified JMF? I am
>> thinking about having separate DataSource and
DastaSink interfaces for
>> example, that would be created and mapped to calls
through the
>> MediaService. Sympho has already suggested in a
previous mail that
>> CallSession for example turns out to be
inappropriate for jabber/jingle
>> so we are probably going to have an RtpFlow
interface soon, so that's
>> also what was making me think that the MediaService
should generally
>> allow for more flexibility.
> 
> Yeah, that makes sense. It seems counter-intuitive, for
instance, that
> the DataSource for a call is handled by the
MediaControl object and
> the DataSink for a call is handled by CallSessionImpl.

I agree, so we'd have to think of the new interfaces to add.
DataSource
and DataSink seem like an obvious choice, though I am not
sure what
exactly are the methods that they should contain.

I also wanted to simplify the CallSession and have all the
sdp
decoding/encoding functionality in a separate place.

> 
>> * MediaServiceImpl
>>
>> -- This one is _very_important_. Unless I've missed
something, it seems
>> to me that when defining call sink and media
control mappings you are
>> not doing anything to make sure you remove them
when the call ends. This
>> means that you'll keep a reference to them even
after they are no longer
>> relevant and both the call, and the
mediacontrol/data sink will never
>> get garbage collected. Could you please fix this?
> 
> Look at the CallEnded() method within Mailbox.java.
That's where I'm
> calling MediaService.unsetCallDataSink() and
> MediaService.unsetCallDataSource(). Those two methods
release the
> mappings in question. Let me know if there's anything
else that needs
> to be done to make sure that the GC takes care of those
objects.

Oh yes true! Sorry I missed it.

Do you think that it would be a good idea to also add a call
listener
inside the media service itself in order to make sure that
we remove the
call, even if the plugin (that might other than the mailbox)
is not
doing it?

>> * CallSessionImpl
>>
>> -- I've removed the video handling code in here.
Had you tested it? I
>> have trouble understanding how it works, provided
you hard set the file
>> type to WAV, and  weren't there any problems coming
from the fact that
>> you write to the same file from 2 different
streams/threads?
> 
> I think what you are looking at is the code that used
to handle the
> video display (not my code). I think this code got
replaced somewhere
> along the line and I forgot to keep it out of the
patch. sorry.

I am talking about line 1693-1700 in the patch that look
like this

> +                    logger.info("starting
recording to file: "+dataDestination);
> +                    MediaLocator dest = new
MediaLocator(dataDestination);    
> +                    DataSource ds =
((Processor)player).getDataOutput();
> +                    DataSink sink =
Manager.createDataSink(((Processor)player).getDataOutput(),
dest);
> +                    player.start();
> +                    //do we know the output file's
duration
> +                    StartRecording record = new
StartRecording(sink);
> +                    record.run();

as well as the StartRecording thread. Don't they record in
the same file
as the audio .... or am I missing something?


>> So I guess that's it, I'd really appreciate it if
you could make sure
>> that everything still works after my changes.
> 
> Actually... your changes in the
IncomingCallTracker.run() method,
> though they add greatly to readability, cause the
program to infinite
> loop ;p.

... well they are readable aren't they? It should be easy to
find the
problem now .
Seriously, sorry for braking the code. I didn't have the
time to test it. Will be more careful next time.

> I'll put the fix in along with some of the other little
things you
> mentioned in your message and send it in a new patch.
> 
> 
>> Cheers, and thank you for contributing!
>> Emil
>>
> 
> No problem. I'm excited to see my changes merged into
mainline!

Glad you are!

By the way, are you planning on writing some user interface
that would
alert users when they have new messages, and allow them to
play, store
and delete them?

Offering the possibility to record a new outgoing message
through SIP
Communicator, as well as playing the existing one also seems
like a
useful feature. Interested in taking this up?

Cheers
Emil
> 
>> Ryan Ricard wrote:
>>> Well, here it is. Attached is the code from my
Summer of Code project
>>> for the mailbox.
>>>
>>> First, a few notes on the attachments:
>>> -I think I did the diff syntax right, but let
me know if I flubbed
>>> something up.
>>> -The binary files (one icon and one sound file)
are included in a
>>> separate archive so as not to ugly up the
patch.
>>>
>>> Also, a few notes on the content:
>>> -The default outgoing message is me. Let me
know if y'all need any
>>> sort of licensing documentation or
copyright-transferral
>>> whachamahoozits.
>>> -The Icon for the Mailbox's entry in the
configuration window is taken
>>> from the Tango Desktop Project. It seemed to
fit and I am lazy (and no
>>> artist!). I originally considered it a
placeholder, but if we want to
>>> use it It seems that we could. Their licensing
conditions are CC
>>> Attribution-Share Alike (available here:
>>> http:/
/creativecommons.org/licenses/by-sa/2.5/). I don't know
how we'd
>>> go about attributing them, so I will leave the
decision as to how to
>>> handle that (or make a new icon entirely) up to
someone with more
>>> know-how.
>>> -There's a few limitations to the Mailbox that
are probably worth
>>> mentioning. I don't do any sort of
compatibility checking on the
>>> user's choice of an outgoing message, mostly
because JMF is a bit of a
>>> quagmire already in this area and we are
planning a migration to FMJ
>>> which might change the list of compatible
formats even more. I highly
>>> recommend testing a file in JMStudio before you
use it as your
>>> outgoing message
>>> -Incoming messages are now just Audio. This
one's high on my to-do list.
>>>
>>> Other than that, things should pretty much work
as expected. I'd
>>> really like to continue contributing to the
project even after the
>>> Summer comes to a close, so please let me know
if there are some
>>> additional Mailbox features that are highly
desirable.
>>>
>>>
>>>
>>>
------------------------------------------------------------
------------
>>>
>>>
------------------------------------------------------------
---------
>>> To unsubscribe, e-mail: dev-unsubscribesip-communicator.dev.java.net
>>> For additional commands, e-mail: dev-helpsip-communicator.dev.java.net
>>
------------------------------------------------------------
---------
>> To unsubscribe, e-mail: dev-unsubscribesip-communicator.dev.java.net
>> For additional commands, e-mail: dev-helpsip-communicator.dev.java.net
>>
>>
> 
> 

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesip-communicator.dev.java.net
For additional commands, e-mail: dev-helpsip-communicator.dev.java.net

Hi again Ryan,

(inline)

Emil Ivov wrote:
>>> -- I've removed the video handling code in
here. Had you tested it? I
>>> have trouble understanding how it works,
provided you hard set the file
>>> type to WAV, and  weren't there any problems
coming from the fact that
>>> you write to the same file from 2 different
streams/threads?
>> I think what you are looking at is the code that
used to handle the
>> video display (not my code). I think this code got
replaced somewhere
>> along the line and I forgot to keep it out of the
patch. sorry.
> 
> I am talking about line 1693-1700 in the patch that
look like this
> 
>> +                    logger.info("starting
recording to file: "+dataDestination);
>> +                    MediaLocator dest = new
MediaLocator(dataDestination);    
>> +                    DataSource ds =
((Processor)player).getDataOutput();
>> +                    DataSink sink =
Manager.createDataSink(((Processor)player).getDataOutput(),
dest);
>> +                    player.start();
>> +                    //do we know the output file's
duration
>> +                    StartRecording record = new
StartRecording(sink);
>> +                    record.run();
> 
> as well as the StartRecording thread. Don't they record
in the same file
> as the audio .... or am I missing something?

OK, forget what I said. I just figured it out, and it was
silly of me
not to commit because otherwise there's no recording. Sorry.
I am doing
so right now. I still changed one or two things, and one or
two others
would need your attention:

* I renamed the start recording thread to - RecordInitiator
* Made sure that the method called in order to start the
record is
start() and not run() (was there a reason why you didn't
want this
executed in a separate thread?)
* Right now this works in a very mailbox oriented way. What
I mean is
that all we do in the MediaService is say that a certain
call would like
to write and read from custom locations (files) instead of
the standard
ones (sound cart and screen).

Then suddenly the CallSessionImpl class somehow knows that
it does not
have to record media until there's no more content in the
outgoing data
source .

For a while I was actually tempted to remove the record
initiator thread
and have recording start right away, but decided not to do
so now and
avoid messing something else up. So, it's your call.
However, if you
decide to keep it, we'd definitely have to come up with a
mechanism that
explicitly requires the mailbox plugin to specify the
delay.

If we go for creating separate DataSrouce and DataSink
interfaces in the
media service, for example, we could make the DataSource
accept
listeners and notify them when it has reached the end of its
content. At
this point the mailbox could call a method on the DataSink
that would
allow it to start recording. This should be pretty
straightforward to
implement too. Does it make sense to you?

Along the same lines, we should probably also explicitly
specify, rather
than assuming it as default behavior, that we  want the
custom file data
source not to loop and that we want SIP Communicator to stop
once it has
reached its end.

So, what do you think about all this?

Emil




> 
>>> So I guess that's it, I'd really appreciate it
if you could make sure
>>> that everything still works after my changes.
>> Actually... your changes in the
IncomingCallTracker.run() method,
>> though they add greatly to readability, cause the
program to infinite
>> loop ;p.
> 
> ... well they are readable aren't they? It should be
easy to find the
> problem now .
Seriously, sorry for braking the code. I didn't have the
> time to test it. Will be more careful next time.
> 
>> I'll put the fix in along with some of the other
little things you
>> mentioned in your message and send it in a new
patch.
>>
>>
>>> Cheers, and thank you for contributing!
>>> Emil
>>>
>> No problem. I'm excited to see my changes merged
into mainline!
> 
> Glad you are!
> 
> By the way, are you planning on writing some user
interface that would
> alert users when they have new messages, and allow them
to play, store
> and delete them?
> 
> Offering the possibility to record a new outgoing
message through SIP
> Communicator, as well as playing the existing one also
seems like a
> useful feature. Interested in taking this up?
> 
> Cheers
> Emil
>>> Ryan Ricard wrote:
>>>> Well, here it is. Attached is the code from
my Summer of Code project
>>>> for the mailbox.
>>>>
>>>> First, a few notes on the attachments:
>>>> -I think I did the diff syntax right, but
let me know if I flubbed
>>>> something up.
>>>> -The binary files (one icon and one sound
file) are included in a
>>>> separate archive so as not to ugly up the
patch.
>>>>
>>>> Also, a few notes on the content:
>>>> -The default outgoing message is me. Let me
know if y'all need any
>>>> sort of licensing documentation or
copyright-transferral
>>>> whachamahoozits.
>>>> -The Icon for the Mailbox's entry in the
configuration window is taken
>>>> from the Tango Desktop Project. It seemed
to fit and I am lazy (and no
>>>> artist!). I originally considered it a
placeholder, but if we want to
>>>> use it It seems that we could. Their
licensing conditions are CC
>>>> Attribution-Share Alike (available here:
>>>> http:/
/creativecommons.org/licenses/by-sa/2.5/). I don't know
how we'd
>>>> go about attributing them, so I will leave
the decision as to how to
>>>> handle that (or make a new icon entirely)
up to someone with more
>>>> know-how.
>>>> -There's a few limitations to the Mailbox
that are probably worth
>>>> mentioning. I don't do any sort of
compatibility checking on the
>>>> user's choice of an outgoing message,
mostly because JMF is a bit of a
>>>> quagmire already in this area and we are
planning a migration to FMJ
>>>> which might change the list of compatible
formats even more. I highly
>>>> recommend testing a file in JMStudio before
you use it as your
>>>> outgoing message
>>>> -Incoming messages are now just Audio. This
one's high on my to-do list.
>>>>
>>>> Other than that, things should pretty much
work as expected. I'd
>>>> really like to continue contributing to the
project even after the
>>>> Summer comes to a close, so please let me
know if there are some
>>>> additional Mailbox features that are highly
desirable.
>>>>
>>>>
>>>>
>>>>
------------------------------------------------------------
------------
>>>>
>>>>
------------------------------------------------------------
---------
>>>> To unsubscribe, e-mail: dev-unsubscribesip-communicator.dev.java.net
>>>> For additional commands, e-mail:
dev-helpsip-communicator.dev.java.net
>>>
------------------------------------------------------------
---------
>>> To unsubscribe, e-mail: dev-unsubscribesip-communicator.dev.java.net
>>> For additional commands, e-mail: dev-helpsip-communicator.dev.java.net
>>>
>>>
>>
> 
>
------------------------------------------------------------
---------
> To unsubscribe, e-mail: dev-unsubscribesip-communicator.dev.java.net
> For additional commands, e-mail: dev-helpsip-communicator.dev.java.net
> 
> 

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesip-communicator.dev.java.net
For additional commands, e-mail: dev-helpsip-communicator.dev.java.net