<div dir="ltr"><div>Hi Rémi and all,</div><div><br></div><div>Any more idea about this comment whether to patch the demux or the es_out module?</div><div><br></div><div>Thank you again</div><div><br></div><div>Goldy</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Mar 19, 2015 at 6:13 AM, Goldy Liang <span dir="ltr"><<a href="mailto:goldyliang@gmail.com" target="_blank">goldyliang@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div dir="ltr">Hi Rémi,<div><br></div><div>Thanks for your comment. Let's talk about patching EsOutSend first. I did not consider that way in the before and thanks for pointing that out. </div><div><br></div><div>I just had a glance how EsOutSend works, but I don't think it is good to implement there. Looks like EsOutSend would be called by all kinds of modules of demuxer or access. In EsOutSend, if I want to exact the time for subtitle, I have to check whether it is sent by the subtitle modules. And looks like each subtitle demux will send the data anyway no matter if it is selected or default. Even if we can identify the subtitle data and save it, it all depends how much data has been sent so as to navigate based on that. For example, if the data of next sentence not sent yet, we can not jump forward one sentence; or if the user just changes the slider bar and jump to a specific position, we may be missing the time data for the previous sentence, etc. And another problem is where to save the time data and how the input thread can access it. In a word, it looks complicate and not good to me.<br></div><div><br></div><div>On the other hand, by controlling the demux, it is more simple and provides more flexibility for future development. I have more features in my mind about language learning, for example, we can show all subtitles in a separate window so that the user can glance and jump to any sentence he wants. This requires the interface module to be able to access the whole subtitle data instead of just the time of prev/next sentence. I would like to make the subtitle demux to clone and dump the whole subtitle data through the input thread control to the interface. That would be consistent with the current mechanism that the subtitle demux reports the timestamps of prev/cur/next sentences to input thread then to interface. If we do that in es_out, I think it would not be possible to allow the interface to access the subtitle data freely.</div><div><br></div><div>Please let me know your point about this. For the other comments, yes they are valid and I can address them.</div><div><br></div><div>Thank you.</div><span class="HOEnZb"><font color="#888888"><div>Goldy</div></font></span></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 18, 2015 at 6:05 PM, Rémi Denis-Courmont <span dir="ltr"><<a href="mailto:remi@remlab.net" target="_blank">remi@remlab.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid">Le mercredi 18 mars 2015, 00:53:07 Goldy Liang a écrit :<br>
> ---<br>
> include/vlc_demux.h | 6 ++<br>
> include/vlc_input.h | 8 +++<br>
> modules/demux/subtitle.c | 159<br>
> +++++++++++++++++++++++++++++++++++++++++++++<br>
<br>
If there is a need to patch the subtitle demuxer, then it might be preferable<br>
to split that to a third patch. However I am not sure patching demuxers is at<br>
all necessary anymore.<br>
<br>
> src/input/control.c | 35 ++++++++++<br>
> src/input/event.c | 7 ++<br>
> src/input/event.h | 1 +<br>
> src/input/input.c | 8 +++<br>
> src/input/input_internal.h | 3 +<br>
> 8 files changed, 227 insertions(+)<br>
><br>
> diff --git a/include/vlc_demux.h b/include/vlc_demux.h<br>
> index 93197ad..99d7d85 100644<br>
> --- a/include/vlc_demux.h<br>
> +++ b/include/vlc_demux.h<br>
> @@ -183,6 +183,12 @@ enum demux_query_e<br>
> DEMUX_NAV_DOWN, /* res=can fail */<br>
> DEMUX_NAV_LEFT, /* res=can fail */<br>
> DEMUX_NAV_RIGHT, /* res=can fail */<br>
> +<br>
> + DEMUX_IS_SUBTITLE_SEEKABLE,<br>
> + DEMUX_GET_SUBTITLE_CUR_SENTENCE_TIME,<br>
<br>
Did you try to save the start and end time stamps of the last and previous<br>
subtitles of the active text track directly in the elementary stream output<br>
(EsOutSend() in src/input/es_out.c)? You would not need to keep track of the<br>
active subtitle, and you would not need to extend demuxer-specific changes.<br>
<br>
> + DEMUX_GET_SUBTITLE_NEXT_SENTENCE_TIME,<br>
> + DEMUX_GET_SUBTITLE_PREV_SENTENCE_TIME,<br>
> +<br>
> };<br>
><br>
> VLC_API int demux_vaControlHelper( stream_t *, int64_t i_start, int64_t<br>
> i_end, int64_t i_bitrate, int i_align, int i_query, va_list args );<br>
> diff --git a/include/vlc_input.h b/include/vlc_input.h<br>
> index c18bea6..a774acc 100644<br>
> --- a/include/vlc_input.h<br>
> +++ b/include/vlc_input.h<br>
> @@ -392,6 +392,9 @@ typedef enum input_event_type_e<br>
> /* A vout_thread_t object has been created/deleted by *the input* */<br>
> INPUT_EVENT_VOUT,<br>
><br>
> + /* A default subtitle data has been loaded */<br>
> + INPUT_EVENT_DEFAULT_SUBTITLE_LOADED,<br>
> +<br>
> } input_event_type_e;<br>
><br>
> /**<br>
> @@ -478,6 +481,11 @@ enum input_query_e<br>
> /* External clock managments */<br>
> INPUT_GET_PCR_SYSTEM, /* arg1=mtime_t *, arg2=mtime_t * res=can<br>
> fail */<br>
> INPUT_MODIFY_PCR_SYSTEM,/* arg1=int absolute, arg2=mtime_t res=can<br>
> fail */<br>
> +<br>
> + INPUT_GET_CUR_SENTENCE_TIME,<br>
> + INPUT_GET_NEXT_SENTENCE_TIME,<br>
> + INPUT_GET_PREV_SENTENCE_TIME,<br>
> + INPUT_GET_SUBTITLE_SEEKABLE,<br>
> };<br>
><br>
> /** @}*/<br>
> diff --git a/modules/demux/subtitle.c b/modules/demux/subtitle.c<br>
> index d8b6dc5..380242d 100644<br>
> --- a/modules/demux/subtitle.c<br>
> +++ b/modules/demux/subtitle.c<br>
> @@ -226,6 +226,25 @@ static int Control( demux_t *, int, va_list );<br>
> static void Fix( demux_t * );<br>
> static char * get_language_from_filename( const char * );<br>
><br>
> +/*<br>
> + * Get the subtitle sentence at a specific time<br>
> + * @param i_subtitles total number of subtitles regarding to *subtitle<br>
> + * @param *subtitle pointer of array of the subtitle data<br>
> + * @param i_time the specific time (in microsecond)<br>
> + * @param i_subtitle return the number of subtitle sentence<br>
> + * -1 if before the first subtitle<br>
> + * 0 ~ i_subtitles-1 if falls into the time<br>
> + * between the start of this sentence and the start of the next (or<br>
> the end time)<br>
> + * @param b_between return whether the time is between two sentence.<br>
> + * true if between the i_subtitle and the next one<br>
> + * false if not between, but falls in the time of i_subtitle.<br>
> + */<br>
> +static void getSubtitleAtTime (int i_subtitles,<br>
> + subtitle_t *subtitle,<br>
> + int64_t i_time,<br>
> + int * i_subtitle,<br>
> + bool * b_between);<br>
> +<br>
> /**************************************************************************<br>
> *** * Module initializer<br>
><br>
> ***************************************************************************<br>
> **/ @@ -590,6 +609,74 @@ static void Close( vlc_object_t *p_this )<br>
> free( p_sys );<br>
> }<br>
><br>
> +/*<br>
> + * Get the subtitle sentence at a specific time<br>
> + * @param i_subtitles total number of subtitles regarding to *subtitle<br>
> + * @param *subtitle pointer of array of the subtitle data<br>
> + * @param i_time the specific time (in microsecond)<br>
> + * @param i_subtitle return the number of subtitle sentence<br>
> + * -1 if before the first subtitle<br>
> + * 0 ~ i_subtitles-1 if falls into the time<br>
> + * between the start of this sentence and the start of the next (or<br>
> the end time)<br>
> + * @param b_between return whether the time is between two sentence.<br>
> + * true if between the i_subtitle and the next one<br>
> + * false if not between, but falls in the time of i_subtitle.<br>
> + */<br>
> +static void getSubtitleAtTime (int i_subtitles,<br>
> + subtitle_t *subtitle,<br>
> + int64_t i_time,<br>
> + int * i_subtitle,<br>
> + bool * b_between)<br>
> +{<br>
> + if (!subtitle || i_subtitles<=0)<br>
> + {<br>
> + *i_subtitle = -1; *b_between = false;<br>
> + return;<br>
> + }<br>
> +<br>
> + // binary search<br>
> + int ll=0, rr=i_subtitles-1;<br>
> +<br>
> + while (ll<=rr)<br>
> + {<br>
> + int mm= (ll+rr) / 2;<br>
> +<br>
> + if ( i_time < subtitle[mm].i_start)<br>
> + {<br>
> + // if the timestamp is before the start of the middle node<br>
> + // continue the search in the lower half<br>
> + rr = mm-1;<br>
> + } else if ( mm<rr && i_time >= subtitle[mm+1].i_start)<br>
> + {<br>
> + // if the middle node is not the right node, and the timestamp<br>
> + // is after the start of the node next to the middle<br>
> + // continue the search in the higher half<br>
> + ll = mm+1;<br>
> +<br>
> + } else<br>
> + {<br>
> + // otherwise the timestamp is between<br>
> + // the start of #mm and the start of #mm+1<br>
> + *i_subtitle = mm;<br>
> + *b_between = i_time > subtitle[mm].i_stop;<br>
> + return;<br>
> + }<br>
> + }<br>
> +<br>
> + if (rr<0)<br>
> + {<br>
> + // timestamp before the first subtitle sentence<br>
> + *i_subtitle = -1; *b_between = true;<br>
> + }<br>
> + else<br>
> + {<br>
> + //should not be here<br>
> + *i_subtitle = -1; *b_between = true;<br>
> + }<br>
<br>
Did you try to use the built-in bsearch()?<br>
<br>
> +}<br>
> +<br>
> +<br>
> +<br>
> /**************************************************************************<br>
> *** * Control:<br>
><br>
> ***************************************************************************<br>
> **/ @@ -678,6 +765,78 @@ static int Control( demux_t *p_demux, int i_query,<br>
> va_list args )<br>
> case DEMUX_CAN_RECORD:<br>
> return VLC_EGENERIC;<br>
><br>
> + case DEMUX_IS_SUBTITLE_SEEKABLE:<br>
> + {<br>
> + bool *b = (bool*) va_arg (args, bool*);<br>
> + *b = true;<br>
> + return VLC_SUCCESS;<br>
> + }<br>
> + case DEMUX_GET_SUBTITLE_CUR_SENTENCE_TIME:<br>
> + {<br>
> + int64_t* pstart = (int64_t*)va_arg( args, int64_t * );<br>
> + int64_t* pend = (int64_t*)va_arg( args, int64_t * );<br>
> +<br>
> + bool b;<br>
> +<br>
> + int64_t i_time = var_GetTime( p_demux->p_input, "time" );<br>
> + int i_subtitle;<br>
<br>
In principles, p_demux->p_input can be NULL, so you should not blindly assume<br>
you can call var_GetTime() here. More generally, I may sound like a broken<br>
record, but it is not advisable to manipulate the input from the demuxer, not<br>
even as read-only.<br>
If nothing else, you could probably call var_GetTime() in input.c and pass the<br>
result as a parameter.<br>
<br>
That said, if at all possible, I think patching the ES output is even better.<br>
<br>
> +<br>
> + getSubtitleAtTime (p_sys->i_subtitles,<br>
> + p_sys->subtitle, i_time, &i_subtitle, &b);<br>
> + *pstart = p_sys->subtitle[i_subtitle].i_start;<br>
> + *pend = p_sys->subtitle[i_subtitle].i_stop;<br>
> +<br>
> + return VLC_SUCCESS;<br>
> + }<br>
> +<br>
> + case DEMUX_GET_SUBTITLE_NEXT_SENTENCE_TIME:<br>
> + {<br>
> + int64_t* pstart = (int64_t*)va_arg( args, int64_t * );<br>
> + int64_t* pend = (int64_t*)va_arg( args, int64_t * );<br>
> +<br>
> + bool b;<br>
> +<br>
> + int64_t i_time = var_GetTime( p_demux->p_input, "time" );<br>
<br>
Ditto.<br>
<br>
> + int i_subtitle;<br>
> +<br>
> + getSubtitleAtTime (p_sys->i_subtitles,<br>
> + p_sys->subtitle, i_time, &i_subtitle, &b);<br>
> +<br>
> + if (i_subtitle < p_sys->i_subtitles - 1)<br>
> + i_subtitle ++;<br>
> +<br>
> + *pstart = p_sys->subtitle[i_subtitle].i_start;<br>
> + *pend = p_sys->subtitle[i_subtitle].i_stop;<br>
> +<br>
> + return VLC_SUCCESS;<br>
> + }<br>
> +<br>
> + case DEMUX_GET_SUBTITLE_PREV_SENTENCE_TIME:<br>
> + {<br>
> + int64_t* pstart = (int64_t*)va_arg( args, int64_t * );<br>
> + int64_t* pend = (int64_t*)va_arg( args, int64_t * );<br>
> +<br>
> + bool b;<br>
> +<br>
> + int64_t i_time = var_GetTime( p_demux->p_input, "time" );<br>
<br>
Ditto.<br>
<br>
> + int i_subtitle;<br>
> +<br>
> + getSubtitleAtTime (p_sys->i_subtitles,<br>
> + p_sys->subtitle, i_time, &i_subtitle, &b);<br>
> +<br>
> + /* If currently between two sentence (b=true),<br>
> + * just jump to the "current" sentence before the break;<br>
> + * Otherwise<br>
> + * jump to the sentence before<br>
> + */<br>
> + if (i_subtitle > 0 && !b)<br>
> + i_subtitle --;<br>
> +<br>
> + *pstart = p_sys->subtitle[i_subtitle].i_start;<br>
> + *pend = p_sys->subtitle[i_subtitle].i_stop;<br>
> +<br>
> + return VLC_SUCCESS;<br>
> + }<br>
> default:<br>
> msg_Err( p_demux, "unknown query %d in subtitle control",<br>
> i_query );<br>
> return VLC_EGENERIC;<br>
> diff --git a/src/input/control.c b/src/input/control.c<br>
> index 2e4633f..c8296a0 100644<br>
> --- a/src/input/control.c<br>
> +++ b/src/input/control.c<br>
> @@ -35,6 +35,7 @@<br>
> #include "event.h"<br>
> #include "resource.h"<br>
> #include "es_out.h"<br>
> +#include "demux.h"<br>
><br>
><br>
> static void UpdateBookmarksOption( input_thread_t * );<br>
> @@ -494,6 +495,40 @@ int input_vaControl( input_thread_t *p_input, int<br>
> i_query, va_list args )<br>
> return es_out_ControlModifyPcrSystem( p_input->p-<br>
><br>
> >p_es_out_display, b_absolute, i_system );<br>
><br>
> }<br>
><br>
> + case INPUT_GET_CUR_SENTENCE_TIME:<br>
> + case INPUT_GET_NEXT_SENTENCE_TIME:<br>
> + case INPUT_GET_PREV_SENTENCE_TIME:<br>
> + {<br>
> + input_source_t * p_sub = p_input->p->p_def_sub;<br>
<br>
p_def_sub is written in the input thread. You cannot blindly read it from<br>
another thread.<br>
<br>
> +<br>
> + if (!p_sub)<br>
> + return VLC_EGENERIC;<br>
> +<br>
> + int64_t* pstart = (int64_t*)va_arg( args, int64_t * );<br>
> + int64_t* pend = (int64_t*)va_arg( args, int64_t * );<br>
> +<br>
> + if (!demux_Control (p_sub->p_demux ,<br>
> + DEMUX_GET_SUBTITLE_CUR_SENTENCE_TIME +<br>
> + i_query - INPUT_GET_CUR_SENTENCE_TIME,<br>
> + pstart, pend))<br>
> + return VLC_SUCCESS;<br>
> + else<br>
> + return VLC_EGENERIC;<br>
<br>
This assumes that the subtitle demux is re-entrant - which it is not.<br>
<br>
Compare INPUT_ADD_SUBTITLE / INPUT_CONTROL_ADD_SUBTITLE,<br>
<br>
> + }<br>
> + case INPUT_GET_SUBTITLE_SEEKABLE:<br>
> + {<br>
> + bool * p_seekable = (bool*)va_arg( args, bool * );<br>
> + input_source_t * p_sub = p_input->p->p_def_sub;<br>
> +<br>
> + *p_seekable = false;<br>
> +<br>
> + if (p_sub)<br>
> + demux_Control (p_sub->p_demux,<br>
> + DEMUX_IS_SUBTITLE_SEEKABLE,<br>
> + p_seekable);<br>
> +<br>
> + return VLC_SUCCESS;<br>
> + }<br>
<br>
Ditto.<br>
<br>
> default:<br>
> msg_Err( p_input, "unknown query in input_vaControl" );<br>
> return VLC_EGENERIC;<br>
> diff --git a/src/input/event.c b/src/input/event.c<br>
> index e32ca8a..f887021 100644<br>
> --- a/src/input/event.c<br>
> +++ b/src/input/event.c<br>
> @@ -191,6 +191,13 @@ void input_SendEventCache( input_thread_t *p_input,<br>
> double f_level )<br>
> Trigger( p_input, INPUT_EVENT_CACHE );<br>
> }<br>
><br>
> +/* Send a signal notifying a default subtitle loaded */<br>
> +void input_SendEventDefSubLoaded( input_thread_t *p_input )<br>
> +{<br>
> + Trigger( p_input, INPUT_EVENT_DEFAULT_SUBTITLE_LOADED);<br>
> +}<br>
> +<br>
> +<br>
> /* FIXME: review them because vlc_event_send might be<br>
> * moved inside input_item* functions.<br>
> */<br>
> diff --git a/src/input/event.h b/src/input/event.h<br>
> index e91ad11..4a63661 100644<br>
> --- a/src/input/event.h<br>
> +++ b/src/input/event.h<br>
> @@ -42,6 +42,7 @@ void input_SendEventSeekpoint( input_thread_t *p_input,<br>
> int i_title, int i_seekp<br>
> void input_SendEventSignal( input_thread_t *p_input, double f_quality,<br>
> double f_strength );<br>
> void input_SendEventState( input_thread_t *p_input, int i_state );<br>
> void input_SendEventCache( input_thread_t *p_input, double f_level );<br>
> +void input_SendEventDefSubLoaded( input_thread_t *p_input );<br>
><br>
> /* TODO rename Item* */<br>
> void input_SendEventMeta( input_thread_t *p_input );<br>
> diff --git a/src/input/input.c b/src/input/input.c<br>
> index 7de30ec..2c2a55a 100644<br>
> --- a/src/input/input.c<br>
> +++ b/src/input/input.c<br>
> @@ -369,6 +369,9 @@ static input_thread_t *Create( vlc_object_t *p_parent,<br>
> input_item_t *p_item,<br>
> /* No slave */<br>
> p_input->p->i_slave = 0;<br>
> p_input->p->slave = NULL;<br>
> +<br>
> + /* No default subtitle */<br>
> + p_input->p->p_def_sub = NULL;<br>
><br>
> /* */<br>
> if( p_resource )<br>
> @@ -2982,6 +2985,11 @@ static void input_SubtitleAdd( input_thread_t<br>
> *p_input,<br>
><br>
> es_out_Control( p_input->p->p_es_out_display,<br>
> ES_OUT_SET_ES_DEFAULT_BY_ID, i_id );<br>
> es_out_Control( p_input->p->p_es_out_display, ES_OUT_SET_ES_BY_ID,<br>
> i_id );<br>
> +<br>
> + /* set default subtitle input source and<br>
> + * send the notification event*/<br>
> + p_input->p->p_def_sub = sub;<br>
> + input_SendEventDefSubLoaded(p_input);<br>
> }<br>
> var_FreeList( &list, NULL );<br>
> }<br>
> diff --git a/src/input/input_internal.h b/src/input/input_internal.h<br>
> index fa8c2b1..98fca02 100644<br>
> --- a/src/input/input_internal.h<br>
> +++ b/src/input/input_internal.h<br>
> @@ -129,6 +129,9 @@ struct input_thread_private_t<br>
> int i_slave;<br>
> input_source_t **slave;<br>
><br>
> + /* Default subtitle input source */<br>
> + input_source_t *p_def_sub;<br>
> +<br>
> /* Resources */<br>
> input_resource_t *p_resource;<br>
> input_resource_t *p_resource_private;<br>
<span><font color="#888888"><br>
--<br>
Rémi Denis-Courmont<br>
<a href="http://www.remlab.net/" target="_blank">http://www.remlab.net/</a><br>
<br>
</font></span></blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>