Review Board 1.7.16


Video format treated as audio when removed from the file playback scheduler

Review Request #1580 - Created Nov. 11, 2011 and submitted

Matt Jordan
1.8
ASTERISK-18682
Reviewers
asterisk-dev
dvossel
Asterisk
When audio formats were changed to 64 bits, audio formats bit masks were no longer contiguous.  Previously, a comparison operator could be used with AST_FORMAT_AUDIO_MASK to check if a format was audio - however, since AUDIO_FORMAT_MASK now includes audio formats that have a bit mask value greater than some video formats, this no longer works and a bitwise AND operator must be used.  The check to delete a scheduled audio / video read callback in file.c still used a comparison check.  As a result, regardless of the format, the stream was treated as audio and would not be descheduled properly.  If a video stream was stopped before it reached the end of the file, the read callback would not be removed from the scheduler, and the next time the scheduler was called (which could be on a subsequent audio file), the video callback would be called on an already disposed of stream.

This patch fixes that by properly checking for the audio / video masks.  It also consolidates the closing of the audio / video streams into a single private function to reduce code duplication between ast_closestream and the filestream_destructor.

I also searched through the rest of the codebase for any other improper checking of a format against AUDIO_FORMAT_MASK - while comparison operators are used against AST_FORMAT_AUDIO_MASK in a few other places (such as in while loops), in those locations, the format is still explicitly checked as to whether or not it is audio using a bitwise AND operator.
Tested using a video softphone and h.264.  Tested listening back a recorded video message and using DTMF tones to interrupt the playback multiple times in various locations.  Tested also listening to the recorded video message using audio only.  In all cases the playback works correctly with this patch.

Diff revision 1 (Latest)

  1. /branches/1.8/main/file.c: Loading...
/branches/1.8/main/file.c
Revision 344607 New Change
[20] 286 lines
[+20] [+] static int exts_compare(const char *exts, const char *type)
287
	}
287
	}
288

    
   
288

   
289
	return 0;
289
	return 0;
290
}
290
}
291

    
   
291

   
292
static void filestream_destructor(void *arg)
292
/*! \internal \brief Close the file stream by canceling any pending read / write callbacks */

    
   
293
static void filestream_close(struct ast_filestream *f)
293
{
294
{
294
	struct ast_filestream *f = arg;
Moved to 312

   
295
	int status;
Moved to 313

   
296
	int pid = -1;
Moved to 314

   
297

    
   

   
298
	/* Stop a running stream if there is one */
295
	/* Stop a running stream if there is one */
299
	if (f->owner) {
296
	if (f->owner) {
300
		if (f->fmt->format < AST_FORMAT_AUDIO_MASK) {
297
		if (f->fmt->format & AST_FORMAT_AUDIO_MASK) {
301
			f->owner->stream = NULL;
298
			f->owner->stream = NULL;
302
			AST_SCHED_DEL(f->owner->sched, f->owner->streamid);
299
			AST_SCHED_DEL(f->owner->sched, f->owner->streamid);
303
			ast_settimeout(f->owner, 0, NULL, NULL);
300
			ast_settimeout(f->owner, 0, NULL, NULL);
304
		} else {
301
		} else if (f->fmt->format & AST_FORMAT_VIDEO_MASK) {
305
			f->owner->vstream = NULL;
302
			f->owner->vstream = NULL;
306
			AST_SCHED_DEL(f->owner->sched, f->owner->vstreamid);
303
			AST_SCHED_DEL(f->owner->sched, f->owner->vstreamid);

    
   
304
		} else {

    
   
305
			ast_log(AST_LOG_WARNING, "Unable to schedule deletion of filestream with unsupported type %s\n", f->fmt->name);

    
   
306
		}
307
		}
307
	}
308
	}
308
}

    
   
309

   

    
   
310
static void filestream_destructor(void *arg)

    
   
311
{
Moved from 294

    
   
312
	struct ast_filestream *f = arg;
Moved from 295

    
   
313
	int status;
Moved from 296

    
   
314
	int pid = -1;

    
   
315

   

    
   
316
	/* Stop a running stream if there is one */

    
   
317
	filestream_close(f);

    
   
318

   
309
	/* destroy the translator on exit */
319
	/* destroy the translator on exit */
310
	if (f->trans)
320
	if (f->trans)
311
		ast_translator_free_path(f->trans);
321
		ast_translator_free_path(f->trans);
312

    
   
322

   
313
	if (f->realfilename && f->filename) {
323
	if (f->realfilename && f->filename) {
[+20] [20] 577 lines
[+20] [+] int ast_stream_rewind(struct ast_filestream *fs, off_t ms)
891
}
901
}
892

    
   
902

   
893
int ast_closestream(struct ast_filestream *f)
903
int ast_closestream(struct ast_filestream *f)
894
{
904
{
895
	/* This used to destroy the filestream, but it now just decrements a refcount.
905
	/* This used to destroy the filestream, but it now just decrements a refcount.
896
	 * We need to force the stream to quit queuing frames now, because we might
906
	 * We close the stream in order to quit queuing frames now, because we might
897
	 * change the writeformat, which could result in a subsequent write error, if
907
	 * change the writeformat, which could result in a subsequent write error, if
898
	 * the format is different. */
908
	 * the format is different. */
899

    
   
909
	filestream_close(f);
900
	/* Stop a running stream if there is one */

   
901
	if (f->owner) {

   
902
		if (f->fmt->format < AST_FORMAT_AUDIO_MASK) {

   
903
			f->owner->stream = NULL;

   
904
			AST_SCHED_DEL(f->owner->sched, f->owner->streamid);

   
905
			ast_settimeout(f->owner, 0, NULL, NULL);

   
906
		} else {

   
907
			f->owner->vstream = NULL;

   
908
			AST_SCHED_DEL(f->owner->sched, f->owner->vstreamid);

   
909
		}

   
910
	}

   
911

    
   

   
912
	ao2_ref(f, -1);
910
	ao2_ref(f, -1);
913
	return 0;
911
	return 0;
914
}
912
}
915

    
   
913

   
916

    
   
914

   
[+20] [20] 573 lines
  1. /branches/1.8/main/file.c: Loading...

https://reviewboard.asterisk.org/ runs on a server provided by Digium, Inc. and uses bandwidth donated to the open source Asterisk community by API Digital Communications in Huntsville, AL USA.
Please report problems with this site to asteriskteam@digium.com.