[vlc-devel] [PATCH] stream_filter: httplive: factorize segment read and remove vlc_object_alive
Rémi Denis-Courmont
remi at remlab.net
Mon Apr 20 21:21:26 CEST 2015
Le lundi 20 avril 2015, 21:12:02 Francois Cartegnie a écrit :
> ---
> modules/stream_filter/httplive.c | 168
> ++++++++++++++++++++------------------- 1 file changed, 88 insertions(+),
> 80 deletions(-)
>
> diff --git a/modules/stream_filter/httplive.c
> b/modules/stream_filter/httplive.c index 24b44c4..ec785e2 100644
> --- a/modules/stream_filter/httplive.c
> +++ b/modules/stream_filter/httplive.c
> @@ -1594,13 +1594,20 @@ static int hls_DownloadSegmentData(stream_t *s,
> hls_stream_t *hls, segment_t *se }
>
> mtime_t start = mdate();
> - if (hls_Download(s, segment) != VLC_SUCCESS)
> + int i_ret;
> +
> + mutex_cleanup_push( &segment->lock ); // C0
> + i_ret = hls_Download(s, segment);
> + vlc_cleanup_pop( ); // C0
> +
> + if (i_ret != VLC_SUCCESS)
> {
> msg_Err(s, "downloading segment %d from stream %d failed",
> segment->sequence, *cur_stream);
> vlc_mutex_unlock(&segment->lock);
> - return VLC_EGENERIC;
> + return i_ret;
> }
> +
> mtime_t duration = mdate() - start;
> if (hls->bandwidth == 0 && segment->duration > 0)
> {
> @@ -1609,14 +1616,13 @@ static int hls_DownloadSegmentData(stream_t *s,
> hls_stream_t *hls, segment_t *se }
>
> /* If the segment is encrypted, decode it */
> - if (hls_DecodeSegmentData(s, hls, segment) != VLC_SUCCESS)
> - {
> - vlc_mutex_unlock(&segment->lock);
> - return VLC_EGENERIC;
> - }
> + i_ret = hls_DecodeSegmentData(s, hls, segment);
>
> vlc_mutex_unlock(&segment->lock);
>
> + if(i_ret != VLC_SUCCESS)
> + return i_ret;
> +
> msg_Dbg(s, "downloaded segment %d from stream %d",
> segment->sequence, *cur_stream);
>
> @@ -1642,9 +1648,7 @@ static void* hls_Thread(void *p_this)
> stream_t *s = (stream_t *)p_this;
> stream_sys_t *p_sys = s->p_sys;
>
> - int canc = vlc_savecancel();
> -
> - while (vlc_object_alive(s))
> + for( ;; )
> {
> hls_stream_t *hls = hls_Get(p_sys->hls_stream,
> p_sys->download.stream); assert(hls);
> @@ -1660,6 +1664,7 @@ static void* hls_Thread(void *p_this)
> {
> /* wait */
> vlc_mutex_lock(&p_sys->download.lock_wait);
> + mutex_cleanup_push(&p_sys->download.lock_wait); //CO
> while (((p_sys->download.segment - p_sys->playback.segment > 6)
> || (p_sys->download.segment >= count)) &&
> (p_sys->download.seek == -1))
> @@ -1667,19 +1672,21 @@ static void* hls_Thread(void *p_this)
> vlc_cond_wait(&p_sys->download.wait,
> &p_sys->download.lock_wait); if (p_sys->b_live /*&& (mdate() >=
> p_sys->playlist.wakeup)*/) break;
> - if (!vlc_object_alive(s))
> - break;
> + vlc_testcancel();
Uh? The vlc_cond_wait() two lines above is a cancellation point already.
> }
> + vlc_cleanup_pop( ); //CO
> +
> /* */
> if (p_sys->download.seek >= 0)
> {
> p_sys->download.segment = p_sys->download.seek;
> p_sys->download.seek = -1;
> }
> +
> vlc_mutex_unlock(&p_sys->download.lock_wait);
> }
>
> - if (!vlc_object_alive(s)) break;
> + vlc_testcancel();
It smells like you have sprinkled magic cancel dust with no obvious logic
here.
>
> vlc_mutex_lock(&hls->lock);
> segment_t *segment = segment_GetSegment(hls,
> p_sys->download.segment); @@ -1688,8 +1695,6 @@ static void*
> hls_Thread(void *p_this)
> if ((segment != NULL) &&
> (hls_DownloadSegmentData(s, hls, segment,
> &p_sys->download.stream) != VLC_SUCCESS)) {
> - if (!vlc_object_alive(s)) break;
> -
> if (!p_sys->b_live)
> {
> p_sys->b_error = true;
> @@ -1714,9 +1719,10 @@ static void* hls_Thread(void *p_this)
> vlc_mutex_lock(&p_sys->read.lock_wait);
> vlc_cond_signal(&p_sys->read.wait);
> vlc_mutex_unlock(&p_sys->read.lock_wait);
> +
> + vlc_testcancel();
> }
>
> - vlc_restorecancel(canc);
> return NULL;
> }
>
> @@ -1727,14 +1733,14 @@ static void* hls_Reload(void *p_this)
>
> assert(p_sys->b_live);
>
> - int canc = vlc_savecancel();
> -
> double wait = 1.0;
> - while (vlc_object_alive(s))
> + for ( ;; )
> {
> mtime_t now = mdate();
> if (now >= p_sys->playlist.wakeup)
> {
> + int canc = vlc_savecancel();
> +
> /* reload the m3u8 if there are less than 3 segments what
> aren't downloaded */ if ( ( p_sys->download.segment -
> p_sys->playback.segment < 3 ) && ( hls_ReloadPlaylist(s) != VLC_SUCCESS) )
> @@ -1758,6 +1764,8 @@ static void* hls_Reload(void *p_this)
> wait = 1.0;
> }
>
> + vlc_restorecancel(canc);
> +
> hls_stream_t *hls = hls_Get(p_sys->hls_stream,
> p_sys->download.stream); assert(hls);
>
> @@ -1774,7 +1782,6 @@ static void* hls_Reload(void *p_this)
> mwait(p_sys->playlist.wakeup);
> }
>
> - vlc_restorecancel(canc);
> return NULL;
> }
>
> @@ -1794,7 +1801,7 @@ static int Prefetch(stream_t *s, int *current)
>
> /* Download ~10s worth of segments of this HLS stream if they exist */
> unsigned segment_amount = (0.5f + 10/hls->duration);
> - for (int i = 0; i < __MIN(vlc_array_count(hls->segments),
> segment_amount); i++) + for (unsigned i = 0; i <
> __MIN((unsigned)vlc_array_count(hls->segments), segment_amount); i++) {
> segment_t *segment = segment_GetSegment(hls,
> p_sys->download.segment); if (segment == NULL )
> @@ -1830,92 +1837,86 @@ static int Prefetch(stream_t *s, int *current)
> /**************************************************************************
> ** *
>
> ***************************************************************************
> */ +#define HLS_READ_SIZE 65536
> static int hls_Download(stream_t *s, segment_t *segment)
> {
> stream_sys_t *p_sys = s->p_sys;
> assert(segment);
> + volatile int i_return = VLC_SUCCESS;
>
> vlc_mutex_lock(&p_sys->lock);
> + mutex_cleanup_push(&p_sys->lock); // C0
> while (p_sys->paused)
> vlc_cond_wait(&p_sys->wait, &p_sys->lock);
> - vlc_mutex_unlock(&p_sys->lock);
> + vlc_cleanup_run( ); // C0 vlc_mutex_unlock(&p_sys->lock);
>
> + int i_canc = vlc_savecancel();
> stream_t *p_ts = stream_UrlNew(s, segment->url);
> + vlc_restorecancel(i_canc);
> if (p_ts == NULL)
> return VLC_EGENERIC;
>
> - segment->size = stream_Size(p_ts);
> + int64_t size = stream_Size(p_ts);
> + if (size < 0)
> + size = 0;
>
> - if (segment->size == 0) {
> - int chunk_size = 65536;
> - segment->data = block_Alloc(chunk_size);
> - if (!segment->data)
> - goto nomem;
> - do {
> - if (segment->data->i_buffer - segment->size < chunk_size) {
> - chunk_size *= 2;
> - block_t *p_block = block_Realloc(segment->data, 0,
> segment->data->i_buffer + chunk_size); - if (!p_block) {
> - block_Release(segment->data);
> - segment->data = NULL;
> - goto nomem;
> - }
> - segment->data = p_block;
> - }
> + vlc_cleanup_push( stream_Delete, p_ts ); /* C1 */
>
> - ssize_t length = stream_Read(p_ts, segment->data->p_buffer +
> segment->size, chunk_size); - if (length <= 0) {
> - segment->data->i_buffer = segment->size;
> - break;
> - }
> - segment->size += length;
> - } while (vlc_object_alive(s));
> + volatile unsigned i_total_read = 0;
volatile = bug.
>
> - stream_Delete(p_ts);
> - return VLC_SUCCESS;
> + block_t *p_segment_data = block_Alloc(size ? size : HLS_READ_SIZE);
> + if (!p_segment_data)
> + {
> + i_return = VLC_ENOMEM;
> + goto end;
> }
> -
> - segment->data = block_Alloc(segment->size);
> - if (segment->data == NULL)
> - goto nomem;
> -
> - assert(segment->data->i_buffer == segment->size);
> -
> - ssize_t curlen = 0;
> - do
> + for( ;; )
> {
> /* NOTE: Beware the size reported for a segment by the HLS server
> may not * be correct, when downloading the segment data. Therefore check
> the size * and enlarge the segment data block if necessary.
> */
> - uint64_t size = stream_Size(p_ts);
> - if (size > segment->size)
> + uint64_t i_toread = (size > 0) ? (uint64_t) size - i_total_read:
> HLS_READ_SIZE; +
> + if (i_total_read + i_toread > p_segment_data->i_buffer)
> {
> - msg_Dbg(s, "size changed %"PRIu64, segment->size);
> - block_t *p_block = block_Realloc(segment->data, 0, size);
> - if (p_block == NULL)
> + msg_Dbg(s, "size changed to %"PRIu64, i_total_read + i_toread);
> + block_t *p_realloc_block = block_Realloc(p_segment_data, 0,
> i_total_read + i_toread); + if (p_realloc_block == NULL)
> {
> - block_Release(segment->data);
> - segment->data = NULL;
> - goto nomem;
> + if(p_segment_data)
> + block_Release(p_segment_data);
> + i_return = VLC_ENOMEM;
> + goto end;
> }
> - segment->data = p_block;
> - segment->size = size;
> - assert(segment->data->i_buffer == segment->size);
> - p_block = NULL;
> + p_segment_data = p_realloc_block;
> }
> - ssize_t length = stream_Read(p_ts, segment->data->p_buffer +
> curlen, segment->size - curlen); - if (length <= 0)
> +
> + int i_canc = vlc_savecancel();
> + int i_length = stream_Read(p_ts,
> &p_segment_data->p_buffer[i_total_read], HLS_READ_SIZE); +
> vlc_restorecancel(i_canc);
> +
> + if (i_length <= 0)
> + {
> + if(size > 0 && i_total_read < size)
> + msg_Warn(s, "segment read %"PRIu64"/%"PRIu64, size -
> i_total_read, size ); + p_segment_data->i_buffer = i_total_read;
> break;
> - curlen += length;
> - } while (vlc_object_alive(s));
> + }
>
> - stream_Delete(p_ts);
> - return VLC_SUCCESS;
> + i_total_read += i_length;
> +
> + vlc_cleanup_push( block_Release, p_segment_data ); /* C2 */
> + vlc_testcancel();
> + vlc_cleanup_pop( ); /* C2 */
> + };
>
> -nomem:
> - stream_Delete(p_ts);
> - return VLC_ENOMEM;
> + segment->data = p_segment_data;
> + segment->size = p_segment_data->i_buffer;
> +
> +end:
> + vlc_cleanup_run( ); /* C1 stream_Delete p_ts */
> + return i_return;
> }
>
> /* Read M3U8 file */
> @@ -2134,7 +2135,10 @@ static int Open(vlc_object_t *p_this)
> if (vlc_clone(&p_sys->thread, hls_Thread, s,
> VLC_THREAD_PRIORITY_INPUT)) {
> if (p_sys->b_live)
> + {
> + vlc_cancel(p_sys->reload);
> vlc_join(p_sys->reload, NULL);
> + }
> goto fail_thread;
> }
>
> @@ -2177,6 +2181,7 @@ static void Close(vlc_object_t *p_this)
>
> vlc_mutex_lock(&p_sys->lock);
> p_sys->paused = false;
> + p_sys->b_error = true;
> vlc_cond_signal(&p_sys->wait);
> vlc_mutex_unlock(&p_sys->lock);
>
> @@ -2191,7 +2196,10 @@ static void Close(vlc_object_t *p_this)
> /* */
> if (p_sys->b_live)
> vlc_join(p_sys->reload, NULL);
> +
> + vlc_cancel(p_sys->thread);
> vlc_join(p_sys->thread, NULL);
> +
> vlc_mutex_destroy(&p_sys->download.lock_wait);
> vlc_cond_destroy(&p_sys->download.wait);
>
> @@ -2393,7 +2401,7 @@ static int Read(stream_t *s, void *buffer, unsigned
> int i_read) while (length == 0)
> {
> // In case an error occurred or the stream was closed return 0
> - if (p_sys->b_error || !vlc_object_alive(s))
> + if (p_sys->b_error || s->b_error)
> return 0;
>
> // Lock the mutex before trying to read to avoid a race condition
> with the download thread @@ -2672,7 +2680,7 @@ static int
> segment_Seek(stream_t *s, const uint64_t pos) (p_sys->download.segment <
> count)))
> {
> vlc_cond_wait(&p_sys->download.wait,
> &p_sys->download.lock_wait); - if (!vlc_object_alive(s) ||
> s->b_error) break;
> + if (p_sys->b_error || s->b_error) break;
> }
> vlc_mutex_unlock(&p_sys->download.lock_wait);
Generally, for the sake of maintainability, it is best to leave cancellation
enabled only in one or a few strategical sleeping location and disabled it
everywhere else.
--
Rémi Denis-Courmont
http://www.remlab.net/
More information about the vlc-devel
mailing list