Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ifs log enhancements #4086

Closed
wants to merge 3 commits into from
Closed

ifs log enhancements #4086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9b528d0
ifs log enhancements
krishanifs Sep 23, 2024
9aca497
remove use of _countof
krishanifs Sep 23, 2024
41c0082
added pjsua_create2 with log config
krishanifs Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions pjlib/include/pj/assert.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,44 @@
*
* @param expr The expression to be evaluated.
*/
#ifndef _DEBUG
sauwming marked this conversation as resolved.
Show resolved Hide resolved
#ifndef pj_assert
#include "pj/log.h"
#include <string.h>
#ifdef _WIN32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Windows, it is possible to use MinGW, so need to confirm whether MinGW compiler has _WIN32 and uses \\ instead of / for path separator.

#define __FILENAME__ (strrchr(__FILE__, '\\') ? strrchr(__FILE__, '\\') + 1 : __FILE__)
#else
#define __FILENAME__ (strrchr(__FILE__, '/') ? strrchr(__FILE__, '/') + 1 : __FILE__)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Better avoid macro names using underscore prefixes (read somewhere it maybe reserved for system, sdk, etc), e.g: PJ_ASSERT_FILENAME?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this in #4098 due to a couple of reasons:

  • platform difficulty (as commented above).
  • if we want to extract the filename only and get rid of its path, perhaps we need to do it in the log instead. It has been previously discussed in PJ_LOG_HAS_FILENAME #3889.

#endif
#define pj_assert(expr) \
do { \
if (!(expr)) { PJ_LOG(1,(__FILENAME__, "Assert failed: " #expr)); } \
} while (0)
#endif
#else
sauwming marked this conversation as resolved.
Show resolved Hide resolved
#ifndef pj_assert
# define pj_assert(expr) assert(expr)
#endif
#endif

/**
* @hideinitializer
* for all buils log the message
* Check during debug build that an expression is true. If the expression
* computes to false during run-time, then the program will stop at the
* offending statements.
* For release build, this macro only print message on the log.
* @param expr The expression to be evaluated.
* @param ... file name,The format string for the log message ("config.c", " PJ_VERSION: %s", PJ_VERSION)
*/
#ifndef PJ_ASSERT_LOG
#include "pj/log.h"
#define PJ_ASSERT_LOG(expr,...) \
do { \
if (!(expr)) { PJ_LOG(1,(__VA_ARGS__)); assert(expr); } \
} while (0)

#endif
/**
* @hideinitializer
* If the expression yields false, assertion will be triggered
Expand Down
16 changes: 12 additions & 4 deletions pjlib/src/pj/os_core_win32.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
#else
# define LOG_MUTEX(expr) PJ_LOG(6,expr)
#endif

# define LOG_MUTEX_WARN(expr) PJ_LOG(1,expr)
#define THIS_FILE "os_core_win32.c"

/*
Expand Down Expand Up @@ -1093,10 +1093,18 @@ PJ_DEF(pj_status_t) pj_mutex_lock(pj_mutex_t *mutex)
status = PJ_STATUS_FROM_OS(GetLastError());

#endif
if (status == PJ_SUCCESS)
{
LOG_MUTEX((mutex->obj_name,
(status==PJ_SUCCESS ? "Mutex acquired by thread %s" : "FAILED by %s"),
"Mutex acquired by thread %s",
pj_thread_this()->obj_name));

}
else
{
LOG_MUTEX_WARN((mutex->obj_name,
"FAILED by %s",
pj_thread_this()->obj_name));
}
sauwming marked this conversation as resolved.
Show resolved Hide resolved
#if PJ_DEBUG
if (status == PJ_SUCCESS) {
mutex->owner = pj_thread_this();
Expand Down Expand Up @@ -1296,7 +1304,7 @@ static pj_status_t pj_sem_wait_for(pj_sem_t *sem, unsigned timeout)
LOG_MUTEX((sem->obj_name, "Semaphore acquired by thread %s",
pj_thread_this()->obj_name));
} else {
LOG_MUTEX((sem->obj_name, "Semaphore: thread %s FAILED to acquire",
LOG_MUTEX_WARN((sem->obj_name, "Semaphore: thread %s FAILED to acquire",
pj_thread_this()->obj_name));
}

Expand Down
20 changes: 12 additions & 8 deletions pjmedia/src/pjmedia/silencedet.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,9 +235,11 @@ PJ_DEF(pj_bool_t) pjmedia_silence_det_apply( pjmedia_silence_det *sd,
/* Voiced for long time (>recalc_on_voiced), current
* threshold seems to be too low.
*/
unsigned old = sd->threshold;
sd->threshold = (avg_recent_level + sd->threshold) >> 1;
TRACE_((THIS_FILE,"Re-adjust threshold (in talk burst)"
"to %d", sd->threshold));
if (sd->threshold != old)
TRACE_((THIS_FILE,"%s re-adjust threshold (in talk burst) to %d (was %d)",
sd->objname, sd->threshold, old));

sd->voiced_timer = 0;

Expand All @@ -248,8 +250,8 @@ PJ_DEF(pj_bool_t) pjmedia_silence_det_apply( pjmedia_silence_det *sd,
break;

case STATE_SILENCE:
TRACE_((THIS_FILE,"Starting talk burst (level=%d threshold=%d)",
level, sd->threshold));
TRACE_((THIS_FILE,"%s starting talk burst (level=%d threshold=%d)",
sd->objname, level, sd->threshold));

case STATE_START_SILENCE:
sd->state = STATE_VOICED;
Expand All @@ -271,9 +273,11 @@ PJ_DEF(pj_bool_t) pjmedia_silence_det_apply( pjmedia_silence_det *sd,
switch(sd->state) {
case STATE_SILENCE:
if (sd->silence_timer >= sd->recalc_on_silence) {
unsigned old = sd->threshold;
sd->threshold = avg_recent_level << 1;
TRACE_((THIS_FILE,"Re-adjust threshold (in silence)"
"to %d", sd->threshold));
if (sd->threshold != old)
TRACE_((THIS_FILE,"%s re-adjust threshold (in silence) to %d (was %d)",
sd->objname, sd->threshold, old));

sd->silence_timer = 0;

Expand All @@ -294,8 +298,8 @@ PJ_DEF(pj_bool_t) pjmedia_silence_det_apply( pjmedia_silence_det *sd,
if (sd->silence_timer >= sd->before_silence) {
sd->state = STATE_SILENCE;
sd->threshold = avg_recent_level << 1;
TRACE_((THIS_FILE,"Starting silence (level=%d "
"threshold=%d)", level, sd->threshold));
TRACE_((THIS_FILE,"%s starting silence (level=%d threshold=%d)",
sd->objname, level, sd->threshold));

/* Reset sig_level */
sd->sum_level = avg_recent_level;
Expand Down
2 changes: 1 addition & 1 deletion pjmedia/src/pjmedia/stream.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1546,7 +1546,7 @@ static pj_status_t put_frame_imp( pjmedia_port *port,
pjmedia_rtp_hdr *rtp = (pjmedia_rtp_hdr*) channel->out_pkt;

rtp->m = 1;
PJ_LOG(5,(stream->port.info.name.ptr,"Start talksprut.."));
PJ_LOG(5,(stream->port.info.name.ptr,"Starting talksprut.."));
sauwming marked this conversation as resolved.
Show resolved Hide resolved
}

stream->is_streaming = PJ_TRUE;
Expand Down
10 changes: 9 additions & 1 deletion pjsip/include/pjsua-lib/pjsua.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2468,7 +2468,15 @@ PJ_DECL(pjsua_msg_data*) pjsua_msg_data_clone(pj_pool_t *pool,
* @return PJ_SUCCESS on success, or the appropriate error code.
*/
PJ_DECL(pj_status_t) pjsua_create(void);

/*
* Instantiate pjsua application. Application must call this function before
* calling any other functions, to make sure that the underlying libraries
* are properly initialized. Once this function has returned success,
* application must call pjsua_destroy() before quitting.
*
* @return PJ_SUCCESS on success, or the appropriate error code.
*/
PJ_DECL(pj_status_t) pjsua_create2(const pjsua_logging_config *log_cfg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we can pass the log config to pjsua_init(), why is this new API needed?


/** Forward declaration */
typedef struct pjsua_media_config pjsua_media_config;
Expand Down
4 changes: 2 additions & 2 deletions pjsip/src/pjsip/sip_dialog.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -965,8 +965,8 @@ PJ_DEF(void) pjsip_dlg_dec_lock(pjsip_dialog *dlg)
{
PJ_ASSERT_ON_FAIL(dlg!=NULL, return);

PJ_LOG(6,(dlg->obj_name, "Entering pjsip_dlg_dec_lock(), sess_count=%d",
dlg->sess_count));
PJ_LOG(6,(dlg->obj_name, "Entering pjsip_dlg_dec_lock(), sess_count=%d, tsx_count=%d",
dlg->sess_count, dlg->tsx_count));

pj_assert(dlg->sess_count > 0);
--dlg->sess_count;
Expand Down
1 change: 1 addition & 0 deletions pjsip/src/pjsip/sip_transaction.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1952,6 +1952,7 @@ static void send_msg_callback( pjsip_send_state *send_state,
tsx_update_transport(tsx, send_state->cur_transport);

/* Update remote address. */
pj_assert(tdata->dest_info.cur_addr < (sizeof(tdata->dest_info.addr.entry)/sizeof(tdata->dest_info.addr.entry[0])));
sauwming marked this conversation as resolved.
Show resolved Hide resolved
tsx->addr_len = tdata->dest_info.addr.entry[tdata->dest_info.cur_addr].addr_len;
pj_memcpy(&tsx->addr,
&tdata->dest_info.addr.entry[tdata->dest_info.cur_addr].addr,
Expand Down
5 changes: 4 additions & 1 deletion pjsip/src/pjsua-lib/pjsua_aud.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -683,7 +683,8 @@ pj_status_t pjsua_aud_channel_update(pjsua_call_media *call_med,
PJ_UNUSED_ARG(local_sdp);
PJ_UNUSED_ARG(remote_sdp);

PJ_LOG(4,(THIS_FILE,"Audio channel update.."));
PJ_LOG(4,(THIS_FILE,"Audio channel update for index %d for call %d...",
call_med->idx, call_med->call->index));
pj_log_push_indent();

si->rtcp_sdes_bye_disabled = pjsua_var.media_cfg.no_rtcp_sdes_bye;
Expand Down Expand Up @@ -834,6 +835,8 @@ pj_status_t pjsua_aud_channel_update(pjsua_call_media *call_med,

on_return:
pj_log_pop_indent();
if (status != PJ_SUCCESS)
PJ_LOG(2, (THIS_FILE, "pjsua_aud_channel_update failed"));
sauwming marked this conversation as resolved.
Show resolved Hide resolved
return status;
}

Expand Down
17 changes: 15 additions & 2 deletions pjsip/src/pjsua-lib/pjsua_core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,7 +728,7 @@ PJ_DEF(pj_status_t) pjsua_reconfigure_logging(const pjsua_logging_config *cfg)
pjsua_logging_config_dup(pjsua_var.pool, &pjsua_var.log_cfg, cfg);

/* Redirect log function to ours */
pj_log_set_log_func( &log_writer );
pj_log_set_log_func( (cfg && cfg->cb && !cfg->log_filename.slen) ? cfg->cb : &log_writer );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • We missed cfg != NULL verification, better add that, e.g: PJ_ASSERT_RETURN(cfg, PJ_EINVAL);
  • Re: skipping the log_writer, it may change the behavior of the lib (potential backward compatibility issue), could you specify the reason?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done for the first one, the second one reverted to current code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to pass a different log_writer call back function that write pj logs into our own application logs. how are we going to achieve that without this implementation.

Copy link
Member

@sauwming sauwming Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function log_writer() will call your callback.:
https://github.com/pjsip/pjproject/blob/master/pjsip/src/pjsua-lib/pjsua_core.c#L730

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an optimization we added. it directly call 'cfg->cb' if user has defined that; without calling it through log_writer. But it is OK to stick with current implementation.


/* Set decor */
pj_log_set_decor(pjsua_var.log_cfg.decor);
Expand Down Expand Up @@ -890,15 +890,28 @@ static void init_random_seed(void)
/*
* Instantiate pjsua application.
*/
PJ_DEF(pj_status_t) pjsua_create(void)
PJ_DECL(pj_status_t) pjsua_create(void)
{
return pjsua_create2(NULL);
}
PJ_DEF(pj_status_t) pjsua_create2(const pjsua_logging_config *log_cfg)
{
pj_status_t status;

/* Init pjsua data */
init_data();

if (log_cfg) {
/* Save custom logging config */
pj_memcpy(&pjsua_var.log_cfg, log_cfg, sizeof(*log_cfg));
pj_bzero(&pjsua_var.log_cfg.log_filename, sizeof(pjsua_var.log_cfg.log_filename));
} else {
/* Set default logging settings */
pjsua_logging_config_default(&pjsua_var.log_cfg);
}
pj_log_set_log_func((log_cfg && log_cfg->cb && !log_cfg->log_filename.slen) ? log_cfg->cb : &log_writer);
pj_log_set_decor(pjsua_var.log_cfg.decor);
pj_log_set_level(pjsua_var.log_cfg.level);

/* Init PJLIB: */
status = pj_init();
Expand Down