Re: [asterisk-dev] [Code Review] 3046: framehooks: Re-iterate when frame is changed
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/#review10434 --- Ship it! Ship It! - Joshua Colp On Dec. 13, 2013, 4:28 p.m., Joshua Colp wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/ --- (Updated Dec. 13, 2013, 4:28 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Currently when a frame is changed by a frame hook previous hooks aren't aware of it. This can be a problem when a previous hook reacts to the types of frames that a subsequent hook produces. An example of this would be the fax gateway hook and the PJSIP T.38 hook. Since the fax gateway hook injects T.38 control frames after the T.38 hook, nothing happens. The attached change makes it so if a frame hook produces a different frame the hook iteration loop is restarted, skipping the hook that has produced the frame. Diffs - /branches/12/main/framehook.c 403747 Diff: https://reviewboard.asterisk.org/r/3046/diff/ Testing --- Ran fax tests, confirmed fixed. Thanks, Joshua Colp -- _ -- 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] 3046: framehooks: Re-iterate when frame is changed
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/ --- (Updated Dec. 17, 2013, 6:31 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Repository: Asterisk Description --- Currently when a frame is changed by a frame hook previous hooks aren't aware of it. This can be a problem when a previous hook reacts to the types of frames that a subsequent hook produces. An example of this would be the fax gateway hook and the PJSIP T.38 hook. Since the fax gateway hook injects T.38 control frames after the T.38 hook, nothing happens. The attached change makes it so if a frame hook produces a different frame the hook iteration loop is restarted, skipping the hook that has produced the frame. Diffs - /branches/12/main/framehook.c 403747 Diff: https://reviewboard.asterisk.org/r/3046/diff/ Testing --- Ran fax tests, confirmed fixed. Thanks, Joshua Colp -- _ -- 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] 3046: framehooks: Re-iterate when frame is changed
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/ --- (Updated Dec. 13, 2013, 4:28 p.m.) Review request for Asterisk Developers. Changes --- I've now tweaked the code so framehooks which have provided new frames will not be called again if another framehook produces a modified frame. This prevents a loop from happening where it could have previously bounced back and forth. Repository: Asterisk Description --- Currently when a frame is changed by a frame hook previous hooks aren't aware of it. This can be a problem when a previous hook reacts to the types of frames that a subsequent hook produces. An example of this would be the fax gateway hook and the PJSIP T.38 hook. Since the fax gateway hook injects T.38 control frames after the T.38 hook, nothing happens. The attached change makes it so if a frame hook produces a different frame the hook iteration loop is restarted, skipping the hook that has produced the frame. Diffs (updated) - /branches/12/main/framehook.c 403747 Diff: https://reviewboard.asterisk.org/r/3046/diff/ Testing --- Ran fax tests, confirmed fixed. Thanks, Joshua Colp -- _ -- 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] 3046: framehooks: Re-iterate when frame is changed
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/#review10411 --- Ship it! Ship It! - Mark Michelson On Dec. 13, 2013, 4:28 p.m., Joshua Colp wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/ --- (Updated Dec. 13, 2013, 4:28 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Currently when a frame is changed by a frame hook previous hooks aren't aware of it. This can be a problem when a previous hook reacts to the types of frames that a subsequent hook produces. An example of this would be the fax gateway hook and the PJSIP T.38 hook. Since the fax gateway hook injects T.38 control frames after the T.38 hook, nothing happens. The attached change makes it so if a frame hook produces a different frame the hook iteration loop is restarted, skipping the hook that has produced the frame. Diffs - /branches/12/main/framehook.c 403747 Diff: https://reviewboard.asterisk.org/r/3046/diff/ Testing --- Ran fax tests, confirmed fixed. Thanks, Joshua Colp -- _ -- 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] 3046: framehooks: Re-iterate when frame is changed
On Dec. 9, 2013, 8:02 p.m., Mark Michelson wrote: The loop situation Matt described is possible. When ast_channel_suppress() is called, it results in AST_FRAME_VOICE being turned into AST_FRAME_NULL. If there is a jitter buffer on a channel, then AST_FRAME_NULL gets turned into AST_FRAME_VOICE. While I don't think this will loop infinitely, it may result in the jitter buffer being depleted. Matt Jordan wrote: Well, nuts. A framehook that mutes a channel and a jitter buffer would certainly be interesting. Josh and I had talked a bit about this in #asterisk-dev. While we need to prevent framehooks from getting stuck in a loop, we also will need to avoid undo processing here. I wonder if all framehooks really need to be treated in this fashion. Jitter buffers certainly don't want to be called multiple times - they've already returned a frame that was in the next slot and/or interpolated a frame. Maybe only certain framehooks should be called again if a frame they previously passed on is transformed. I don't think you can really group that into certain types of frames, it's really up to the previous framehooks whether they need to be called again or not. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/#review10339 --- On Dec. 9, 2013, 1:27 a.m., Joshua Colp wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/ --- (Updated Dec. 9, 2013, 1:27 a.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Currently when a frame is changed by a frame hook previous hooks aren't aware of it. This can be a problem when a previous hook reacts to the types of frames that a subsequent hook produces. An example of this would be the fax gateway hook and the PJSIP T.38 hook. Since the fax gateway hook injects T.38 control frames after the T.38 hook, nothing happens. The attached change makes it so if a frame hook produces a different frame the hook iteration loop is restarted, skipping the hook that has produced the frame. Diffs - /branches/12/main/framehook.c 403448 Diff: https://reviewboard.asterisk.org/r/3046/diff/ Testing --- Ran fax tests, confirmed fixed. Thanks, Joshua Colp -- _ -- 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] 3046: framehooks: Re-iterate when frame is changed
On Dec. 9, 2013, 8:02 p.m., Mark Michelson wrote: The loop situation Matt described is possible. When ast_channel_suppress() is called, it results in AST_FRAME_VOICE being turned into AST_FRAME_NULL. If there is a jitter buffer on a channel, then AST_FRAME_NULL gets turned into AST_FRAME_VOICE. While I don't think this will loop infinitely, it may result in the jitter buffer being depleted. Matt Jordan wrote: Well, nuts. A framehook that mutes a channel and a jitter buffer would certainly be interesting. Josh and I had talked a bit about this in #asterisk-dev. While we need to prevent framehooks from getting stuck in a loop, we also will need to avoid undo processing here. I wonder if all framehooks really need to be treated in this fashion. Jitter buffers certainly don't want to be called multiple times - they've already returned a frame that was in the next slot and/or interpolated a frame. Maybe only certain framehooks should be called again if a frame they previously passed on is transformed. Joshua Colp wrote: I don't think you can really group that into certain types of frames, it's really up to the previous framehooks whether they need to be called again or not. To respond to myself... I think that's my beef with framehooks. They aren't granular, you get everything. This means that the core can't make intelligent decisions (such as this). - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/#review10339 --- On Dec. 9, 2013, 1:27 a.m., Joshua Colp wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/ --- (Updated Dec. 9, 2013, 1:27 a.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Currently when a frame is changed by a frame hook previous hooks aren't aware of it. This can be a problem when a previous hook reacts to the types of frames that a subsequent hook produces. An example of this would be the fax gateway hook and the PJSIP T.38 hook. Since the fax gateway hook injects T.38 control frames after the T.38 hook, nothing happens. The attached change makes it so if a frame hook produces a different frame the hook iteration loop is restarted, skipping the hook that has produced the frame. Diffs - /branches/12/main/framehook.c 403448 Diff: https://reviewboard.asterisk.org/r/3046/diff/ Testing --- Ran fax tests, confirmed fixed. Thanks, Joshua Colp -- _ -- 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] 3046: framehooks: Re-iterate when frame is changed
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/#review10339 --- The loop situation Matt described is possible. When ast_channel_suppress() is called, it results in AST_FRAME_VOICE being turned into AST_FRAME_NULL. If there is a jitter buffer on a channel, then AST_FRAME_NULL gets turned into AST_FRAME_VOICE. While I don't think this will loop infinitely, it may result in the jitter buffer being depleted. - Mark Michelson On Dec. 9, 2013, 1:27 a.m., Joshua Colp wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/ --- (Updated Dec. 9, 2013, 1:27 a.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Currently when a frame is changed by a frame hook previous hooks aren't aware of it. This can be a problem when a previous hook reacts to the types of frames that a subsequent hook produces. An example of this would be the fax gateway hook and the PJSIP T.38 hook. Since the fax gateway hook injects T.38 control frames after the T.38 hook, nothing happens. The attached change makes it so if a frame hook produces a different frame the hook iteration loop is restarted, skipping the hook that has produced the frame. Diffs - /branches/12/main/framehook.c 403448 Diff: https://reviewboard.asterisk.org/r/3046/diff/ Testing --- Ran fax tests, confirmed fixed. Thanks, Joshua Colp -- _ -- 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] 3046: framehooks: Re-iterate when frame is changed
On Dec. 9, 2013, 8:02 p.m., Mark Michelson wrote: The loop situation Matt described is possible. When ast_channel_suppress() is called, it results in AST_FRAME_VOICE being turned into AST_FRAME_NULL. If there is a jitter buffer on a channel, then AST_FRAME_NULL gets turned into AST_FRAME_VOICE. While I don't think this will loop infinitely, it may result in the jitter buffer being depleted. Well, nuts. A framehook that mutes a channel and a jitter buffer would certainly be interesting. Josh and I had talked a bit about this in #asterisk-dev. While we need to prevent framehooks from getting stuck in a loop, we also will need to avoid undo processing here. I wonder if all framehooks really need to be treated in this fashion. Jitter buffers certainly don't want to be called multiple times - they've already returned a frame that was in the next slot and/or interpolated a frame. Maybe only certain framehooks should be called again if a frame they previously passed on is transformed. - Matt --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/#review10339 --- On Dec. 9, 2013, 1:27 a.m., Joshua Colp wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/ --- (Updated Dec. 9, 2013, 1:27 a.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Currently when a frame is changed by a frame hook previous hooks aren't aware of it. This can be a problem when a previous hook reacts to the types of frames that a subsequent hook produces. An example of this would be the fax gateway hook and the PJSIP T.38 hook. Since the fax gateway hook injects T.38 control frames after the T.38 hook, nothing happens. The attached change makes it so if a frame hook produces a different frame the hook iteration loop is restarted, skipping the hook that has produced the frame. Diffs - /branches/12/main/framehook.c 403448 Diff: https://reviewboard.asterisk.org/r/3046/diff/ Testing --- Ran fax tests, confirmed fixed. Thanks, Joshua Colp -- _ -- 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] 3046: framehooks: Re-iterate when frame is changed
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/ --- (Updated Dec. 9, 2013, 1:27 a.m.) Review request for Asterisk Developers. Changes --- Incorporated feedback from Matt. Re: Comment about looking at framehook implementations. They all seem to do just what they need to do, and return as rapidly as possible. Repository: Asterisk Description --- Currently when a frame is changed by a frame hook previous hooks aren't aware of it. This can be a problem when a previous hook reacts to the types of frames that a subsequent hook produces. An example of this would be the fax gateway hook and the PJSIP T.38 hook. Since the fax gateway hook injects T.38 control frames after the T.38 hook, nothing happens. The attached change makes it so if a frame hook produces a different frame the hook iteration loop is restarted, skipping the hook that has produced the frame. Diffs (updated) - /branches/12/main/framehook.c 403448 Diff: https://reviewboard.asterisk.org/r/3046/diff/ Testing --- Ran fax tests, confirmed fixed. Thanks, Joshua Colp -- _ -- 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] 3046: framehooks: Re-iterate when frame is changed
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/#review10334 --- Ship it! Ship It! - Matt Jordan On Dec. 9, 2013, 1:27 a.m., Joshua Colp wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3046/ --- (Updated Dec. 9, 2013, 1:27 a.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Currently when a frame is changed by a frame hook previous hooks aren't aware of it. This can be a problem when a previous hook reacts to the types of frames that a subsequent hook produces. An example of this would be the fax gateway hook and the PJSIP T.38 hook. Since the fax gateway hook injects T.38 control frames after the T.38 hook, nothing happens. The attached change makes it so if a frame hook produces a different frame the hook iteration loop is restarted, skipping the hook that has produced the frame. Diffs - /branches/12/main/framehook.c 403448 Diff: https://reviewboard.asterisk.org/r/3046/diff/ Testing --- Ran fax tests, confirmed fixed. Thanks, Joshua Colp -- _ -- 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] 3046: framehooks: Re-iterate when frame is changed
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3046/#review10329
---
/branches/12/main/framehook.c
https://reviewboard.asterisk.org/r/3046/#comment19729
I'd scrap the goto loop.
do {
} while (frame != original_frame)
does the same without being ew.
The other issue here is that two frame hooks could cancel each other out:
* original_frame = foo
* frame hook A returns converts foo to bar
* frame hook A is passed on
* frame hook B converts bar to foo
* repeat... :-(
We could ignore this, but if we ever hit this problem, it will be
incredibly ugly.
The other option is to clone the framehook list before we start the pass.
When a framehook modifies the frame, we remove the framehook from the cloned
list and then re-iterate. That guarantees that a framehook is never called
twice and prevents this situation.
/branches/12/main/framehook.c
https://reviewboard.asterisk.org/r/3046/#comment19728
Blob
My only major fear with this change is that it effectively adds a substantial
amount of processing on every frame. It does guarantee corectness, but I think
it would be worthwhile to take a look at our framehook implementations to
ensure that they aren't doing anything beyond the bare minimum for frames that
they don't otherwise consume.
- Matt Jordan
On Dec. 5, 2013, 5:20 p.m., Joshua Colp wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3046/
---
(Updated Dec. 5, 2013, 5:20 p.m.)
Review request for Asterisk Developers.
Repository: Asterisk
Description
---
Currently when a frame is changed by a frame hook previous hooks aren't aware
of it. This can be a problem when a previous hook reacts to the types of
frames that a subsequent hook produces. An example of this would be the fax
gateway hook and the PJSIP T.38 hook. Since the fax gateway hook injects T.38
control frames after the T.38 hook, nothing happens.
The attached change makes it so if a frame hook produces a different frame
the hook iteration loop is restarted, skipping the hook that has produced the
frame.
Diffs
-
/branches/12/main/framehook.c 403362
Diff: https://reviewboard.asterisk.org/r/3046/diff/
Testing
---
Ran fax tests, confirmed fixed.
Thanks,
Joshua Colp
--
_
-- 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
