Re: [asterisk-dev] [Code Review] 3512: media formats: Convert the translation core over

2014-05-29 Thread Kevin Harwell 

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3512/
---

(Updated May 29, 2014, 2:37 p.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Changes
---

Committed in revision 414875


Bugs: SWP-6725
https://issues.asterisk.org/jira/browse/SWP-6725


Repository: Asterisk


Description
---

Updated the translation core (translate.c) to use the new media format API.


Diffs
-

  team/group/media_formats-reviewed/main/translate.c 413541 
  team/group/media_formats-reviewed/main/format.c 413541 
  team/group/media_formats-reviewed/main/codec.c 413541 
  team/group/media_formats-reviewed/include/asterisk/format.h 413541 

Diff: https://reviewboard.asterisk.org/r/3512/diff/


Testing
---


Thanks,

Kevin Harwell

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3512: media formats: Convert the translation core over

2014-05-29 Thread Mark Michelson 

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3512/#review12002
---

Ship it!


Ship It!

- Mark Michelson


On May 8, 2014, 8:37 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3512/
> ---
> 
> (Updated May 8, 2014, 8:37 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: SWP-6725
> https://issues.asterisk.org/jira/browse/SWP-6725
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Updated the translation core (translate.c) to use the new media format API.
> 
> 
> Diffs
> -
> 
>   team/group/media_formats-reviewed/main/translate.c 413541 
>   team/group/media_formats-reviewed/main/format.c 413541 
>   team/group/media_formats-reviewed/main/codec.c 413541 
>   team/group/media_formats-reviewed/include/asterisk/format.h 413541 
> 
> Diff: https://reviewboard.asterisk.org/r/3512/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3512: media formats: Convert the translation core over

2014-05-08 Thread Kevin Harwell 

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3512/
---

(Updated May 8, 2014, 3:37 p.m.)


Review request for Asterisk Developers.


Changes
---

Updated based upon review feedback.


Bugs: SWP-6725
https://issues.asterisk.org/jira/browse/SWP-6725


Repository: Asterisk


Description
---

Updated the translation core (translate.c) to use the new media format API.


Diffs (updated)
-

  team/group/media_formats-reviewed/main/translate.c 413541 
  team/group/media_formats-reviewed/main/format.c 413541 
  team/group/media_formats-reviewed/main/codec.c 413541 
  team/group/media_formats-reviewed/include/asterisk/format.h 413541 

Diff: https://reviewboard.asterisk.org/r/3512/diff/


Testing
---


Thanks,

Kevin Harwell

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3512: media formats: Convert the translation core over

2014-05-08 Thread Kevin Harwell 


> On May 8, 2014, 1 p.m., Mark Michelson wrote:
> > team/group/media_formats-reviewed/include/asterisk/codec.h, lines 180-189
> > 
> >
> > 1) Document that this function appends the names onto the buf rather 
> > than just setting the input buf to the codec names.
> > 2) Returning an ast_str ** is awkward and unnecessary. If you wanted to 
> > make this return something, a more natural way might be to do something 
> > like:
> > 
> > struct ast_str *ast_codec_get_names(const struct ast_codec *codec, 
> > struct ast_str *buf);
> > 
> > And then require that it gets called like:
> > 
> > str = ast_codec_get_names(codec, str);
> > 
> > As it is, I'm fine with it just returning void (or possibly a pass/fail 
> > int).

This method has been dropped.


> On May 8, 2014, 1 p.m., Mark Michelson wrote:
> > team/group/media_formats-reviewed/main/codec.c, lines 355-364
> > 
> >
> > This won't compile since buf is undeclared.
> > 
> > I can see two ways of fixing this.
> > 1) Switch to using ao2_callback_data() and pass the buf into this 
> > callback.
> > 
> > 2) Since you're doing a straight pointer comparison, will the same 
> > codec ever exist in the container more than once? If not, then instead of 
> > performing an ao2_callback to append codec names to an ast_str, change the 
> > ao2_callback to find the single matching codec and perform the string 
> > append in ast_codec_get_names().

After talking it over with Josh it turns out that there is no need for the name 
lookup at all.  This was an artifact of the old code that did not store the 
name with the codec, so it had to be looked up.   Also Josh mentioned it might 
be helpful to print out the sample rate along with the name, so I I'll add that 
in as well.


> On May 8, 2014, 1 p.m., Mark Michelson wrote:
> > team/group/media_formats-reviewed/main/translate.c, lines 714-715
> > 
> >
> > Does a comparison between different sample rates of signed linear 
> > codecs return AST_FORMAT_CMP_EQUAL? If not, then this functions differently 
> > from the old code.

It doesn't properly check.  I should be able to check against the codec name 
since all slin codecs have the name "slin".


- Kevin


---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3512/#review11854
---


On May 8, 2014, 12:16 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3512/
> ---
> 
> (Updated May 8, 2014, 12:16 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: SWP-6725
> https://issues.asterisk.org/jira/browse/SWP-6725
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Updated the translation core (translate.c) to use the new media format API.
> 
> 
> Diffs
> -
> 
>   team/group/media_formats-reviewed/main/translate.c 413539 
>   team/group/media_formats-reviewed/main/format.c 413539 
>   team/group/media_formats-reviewed/main/codec.c 413539 
>   team/group/media_formats-reviewed/include/asterisk/format.h 413539 
>   team/group/media_formats-reviewed/include/asterisk/codec.h 413539 
> 
> Diff: https://reviewboard.asterisk.org/r/3512/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3512: media formats: Convert the translation core over

2014-05-08 Thread Mark Michelson 

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3512/#review11854
---



team/group/media_formats-reviewed/include/asterisk/codec.h


1) Document that this function appends the names onto the buf rather than 
just setting the input buf to the codec names.
2) Returning an ast_str ** is awkward and unnecessary. If you wanted to 
make this return something, a more natural way might be to do something like:

struct ast_str *ast_codec_get_names(const struct ast_codec *codec, struct 
ast_str *buf);

And then require that it gets called like:

str = ast_codec_get_names(codec, str);

As it is, I'm fine with it just returning void (or possibly a pass/fail 
int).



team/group/media_formats-reviewed/main/codec.c


This won't compile since buf is undeclared.

I can see two ways of fixing this.
1) Switch to using ao2_callback_data() and pass the buf into this callback.

2) Since you're doing a straight pointer comparison, will the same codec 
ever exist in the container more than once? If not, then instead of performing 
an ao2_callback to append codec names to an ast_str, change the ao2_callback to 
find the single matching codec and perform the string append in 
ast_codec_get_names().



team/group/media_formats-reviewed/main/translate.c


Does a comparison between different sample rates of signed linear codecs 
return AST_FORMAT_CMP_EQUAL? If not, then this functions differently from the 
old code.


- Mark Michelson


On May 8, 2014, 5:16 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3512/
> ---
> 
> (Updated May 8, 2014, 5:16 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: SWP-6725
> https://issues.asterisk.org/jira/browse/SWP-6725
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Updated the translation core (translate.c) to use the new media format API.
> 
> 
> Diffs
> -
> 
>   team/group/media_formats-reviewed/main/translate.c 413539 
>   team/group/media_formats-reviewed/main/format.c 413539 
>   team/group/media_formats-reviewed/main/codec.c 413539 
>   team/group/media_formats-reviewed/include/asterisk/format.h 413539 
>   team/group/media_formats-reviewed/include/asterisk/codec.h 413539 
> 
> Diff: https://reviewboard.asterisk.org/r/3512/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3512: media formats: Convert the translation core over

2014-05-08 Thread Joshua Colp 

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3512/#review11851
---

Ship it!


Ship It!

- Joshua Colp


On May 8, 2014, 5:16 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3512/
> ---
> 
> (Updated May 8, 2014, 5:16 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: SWP-6725
> https://issues.asterisk.org/jira/browse/SWP-6725
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Updated the translation core (translate.c) to use the new media format API.
> 
> 
> Diffs
> -
> 
>   team/group/media_formats-reviewed/main/translate.c 413539 
>   team/group/media_formats-reviewed/main/format.c 413539 
>   team/group/media_formats-reviewed/main/codec.c 413539 
>   team/group/media_formats-reviewed/include/asterisk/format.h 413539 
>   team/group/media_formats-reviewed/include/asterisk/codec.h 413539 
> 
> Diff: https://reviewboard.asterisk.org/r/3512/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3512: media formats: Convert the translation core over

2014-05-08 Thread Kevin Harwell 

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3512/
---

(Updated May 8, 2014, 12:16 p.m.)


Review request for Asterisk Developers.


Changes
---

Addressed review findings.  Moved the setting of particular frame details to 
happen on the private structure creation (in newpvt function) as these values 
only really need to be set once.


Bugs: SWP-6725
https://issues.asterisk.org/jira/browse/SWP-6725


Repository: Asterisk


Description
---

Updated the translation core (translate.c) to use the new media format API.


Diffs (updated)
-

  team/group/media_formats-reviewed/main/translate.c 413539 
  team/group/media_formats-reviewed/main/format.c 413539 
  team/group/media_formats-reviewed/main/codec.c 413539 
  team/group/media_formats-reviewed/include/asterisk/format.h 413539 
  team/group/media_formats-reviewed/include/asterisk/codec.h 413539 

Diff: https://reviewboard.asterisk.org/r/3512/diff/


Testing
---


Thanks,

Kevin Harwell

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3512: media formats: Convert the translation core over

2014-05-07 Thread Joshua Colp 

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3512/#review11844
---



team/group/media_formats-reviewed/main/codec.c


This will no longer work because original_id no longer exists, it's been 
removed as part of the legacy format API. What I'd suggest doing is a simple 
pointer comparison. Only 1 structure will ever exist for a codec, it gets used 
everywhere.



team/group/media_formats-reviewed/main/translate.c


Don't create a format here using this method - translator codecs have a 
field called "format" which is the name of a cached format. If that exists use 
it to get the format, and if it does not then fall back to this method.

As well - this function will get called multiple times so ideally only set 
the main frame details up once instead of each time, like was done in the past.

Type, format, mallocd, offset, src, and data.


- Joshua Colp


On May 2, 2014, 2:51 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3512/
> ---
> 
> (Updated May 2, 2014, 2:51 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: SWP-6725
> https://issues.asterisk.org/jira/browse/SWP-6725
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Updated the translation core (translate.c) to use the new media format API.
> 
> 
> Diffs
> -
> 
>   team/group/media_formats-reviewed/main/translate.c 413195 
>   team/group/media_formats-reviewed/main/format.c 413195 
>   team/group/media_formats-reviewed/main/codec.c 413195 
>   team/group/media_formats-reviewed/include/asterisk/translate.h 413195 
>   team/group/media_formats-reviewed/include/asterisk/format.h 413195 
>   team/group/media_formats-reviewed/include/asterisk/codec.h 413195 
> 
> Diff: https://reviewboard.asterisk.org/r/3512/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3512: media formats: Convert the translation core over

2014-05-02 Thread Kevin Harwell 

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3512/
---

(Updated May 2, 2014, 9:51 a.m.)


Review request for Asterisk Developers.


Changes
---

Addressed review findings.


Bugs: SWP-6725
https://issues.asterisk.org/jira/browse/SWP-6725


Repository: Asterisk


Description
---

Updated the translation core (translate.c) to use the new media format API.


Diffs (updated)
-

  team/group/media_formats-reviewed/main/translate.c 413195 
  team/group/media_formats-reviewed/main/format.c 413195 
  team/group/media_formats-reviewed/main/codec.c 413195 
  team/group/media_formats-reviewed/include/asterisk/translate.h 413195 
  team/group/media_formats-reviewed/include/asterisk/format.h 413195 
  team/group/media_formats-reviewed/include/asterisk/codec.h 413195 

Diff: https://reviewboard.asterisk.org/r/3512/diff/


Testing
---


Thanks,

Kevin Harwell

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev

Re: [asterisk-dev] [Code Review] 3512: media formats: Convert the translation core over

2014-05-01 Thread Mark Michelson 

---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3512/#review11808
---



team/group/media_formats-reviewed/main/translate.c


This loop is effectively a no-op since curlen is never set to anything 
within the loop.



team/group/media_formats-reviewed/main/translate.c


This should be

dst_fmt_out = ast_format_copy(bestdst);



team/group/media_formats-reviewed/main/translate.c


This is doing the opposite of what the old code did.



team/group/media_formats-reviewed/main/translate.c


These are the exact same calls twice in a row. I assume one was supposed to 
use AST_MEDIA_TYPE_VIDEO?


- Mark Michelson


On April 30, 2014, 9:22 p.m., Kevin Harwell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3512/
> ---
> 
> (Updated April 30, 2014, 9:22 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: SWP-6725
> https://issues.asterisk.org/jira/browse/SWP-6725
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> Updated the translation core (translate.c) to use the new media format API.
> 
> 
> Diffs
> -
> 
>   team/group/media_formats-reviewed/main/translate.c 413143 
>   team/group/media_formats-reviewed/main/format.c 413143 
>   team/group/media_formats-reviewed/main/codec.c 413143 
>   team/group/media_formats-reviewed/include/asterisk/format.h 413143 
>   team/group/media_formats-reviewed/include/asterisk/codec.h 413143 
> 
> Diff: https://reviewboard.asterisk.org/r/3512/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

-- 
_
-- Bandwidth and Colocation Provided by http://www.api-digital.com --

asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
   http://lists.digium.com/mailman/listinfo/asterisk-dev