[vlc-devel] [PATCH 1/2] Navigation enhancement for language learning
Goldy Liang
goldyliang at gmail.com
Tue Mar 24 23:54:22 CET 2015
Hi Rémi and all,
Any more idea about this comment whether to patch the demux or the es_out
module?
Thank you again
Goldy
On Thu, Mar 19, 2015 at 6:13 AM, Goldy Liang <goldyliang at gmail.com> wrote:
> Hi Rémi,
>
> 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.
>
> 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.
>
> 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.
>
> Please let me know your point about this. For the other comments, yes they
> are valid and I can address them.
>
> Thank you.
> Goldy
>
> On Wed, Mar 18, 2015 at 6:05 PM, Rémi Denis-Courmont <remi at remlab.net>
> wrote:
>
>> Le mercredi 18 mars 2015, 00:53:07 Goldy Liang a écrit :
>> > ---
>> > include/vlc_demux.h | 6 ++
>> > include/vlc_input.h | 8 +++
>> > modules/demux/subtitle.c | 159
>> > +++++++++++++++++++++++++++++++++++++++++++++
>>
>> If there is a need to patch the subtitle demuxer, then it might be
>> preferable
>> to split that to a third patch. However I am not sure patching demuxers
>> is at
>> all necessary anymore.
>>
>> > src/input/control.c | 35 ++++++++++
>> > src/input/event.c | 7 ++
>> > src/input/event.h | 1 +
>> > src/input/input.c | 8 +++
>> > src/input/input_internal.h | 3 +
>> > 8 files changed, 227 insertions(+)
>> >
>> > diff --git a/include/vlc_demux.h b/include/vlc_demux.h
>> > index 93197ad..99d7d85 100644
>> > --- a/include/vlc_demux.h
>> > +++ b/include/vlc_demux.h
>> > @@ -183,6 +183,12 @@ enum demux_query_e
>> > DEMUX_NAV_DOWN, /* res=can fail */
>> > DEMUX_NAV_LEFT, /* res=can fail */
>> > DEMUX_NAV_RIGHT, /* res=can fail */
>> > +
>> > + DEMUX_IS_SUBTITLE_SEEKABLE,
>> > + DEMUX_GET_SUBTITLE_CUR_SENTENCE_TIME,
>>
>> Did you try to save the start and end time stamps of the last and previous
>> subtitles of the active text track directly in the elementary stream
>> output
>> (EsOutSend() in src/input/es_out.c)? You would not need to keep track of
>> the
>> active subtitle, and you would not need to extend demuxer-specific
>> changes.
>>
>> > + DEMUX_GET_SUBTITLE_NEXT_SENTENCE_TIME,
>> > + DEMUX_GET_SUBTITLE_PREV_SENTENCE_TIME,
>> > +
>> > };
>> >
>> > VLC_API int demux_vaControlHelper( stream_t *, int64_t i_start, int64_t
>> > i_end, int64_t i_bitrate, int i_align, int i_query, va_list args );
>> > diff --git a/include/vlc_input.h b/include/vlc_input.h
>> > index c18bea6..a774acc 100644
>> > --- a/include/vlc_input.h
>> > +++ b/include/vlc_input.h
>> > @@ -392,6 +392,9 @@ typedef enum input_event_type_e
>> > /* A vout_thread_t object has been created/deleted by *the input*
>> */
>> > INPUT_EVENT_VOUT,
>> >
>> > + /* A default subtitle data has been loaded */
>> > + INPUT_EVENT_DEFAULT_SUBTITLE_LOADED,
>> > +
>> > } input_event_type_e;
>> >
>> > /**
>> > @@ -478,6 +481,11 @@ enum input_query_e
>> > /* External clock managments */
>> > INPUT_GET_PCR_SYSTEM, /* arg1=mtime_t *, arg2=mtime_t *
>> res=can
>> > fail */
>> > INPUT_MODIFY_PCR_SYSTEM,/* arg1=int absolute, arg2=mtime_t
>> res=can
>> > fail */
>> > +
>> > + INPUT_GET_CUR_SENTENCE_TIME,
>> > + INPUT_GET_NEXT_SENTENCE_TIME,
>> > + INPUT_GET_PREV_SENTENCE_TIME,
>> > + INPUT_GET_SUBTITLE_SEEKABLE,
>> > };
>> >
>> > /** @}*/
>> > diff --git a/modules/demux/subtitle.c b/modules/demux/subtitle.c
>> > index d8b6dc5..380242d 100644
>> > --- a/modules/demux/subtitle.c
>> > +++ b/modules/demux/subtitle.c
>> > @@ -226,6 +226,25 @@ static int Control( demux_t *, int, va_list );
>> > static void Fix( demux_t * );
>> > static char * get_language_from_filename( const char * );
>> >
>> > +/*
>> > + * Get the subtitle sentence at a specific time
>> > + * @param i_subtitles total number of subtitles regarding to *subtitle
>> > + * @param *subtitle pointer of array of the subtitle data
>> > + * @param i_time the specific time (in microsecond)
>> > + * @param i_subtitle return the number of subtitle sentence
>> > + * -1 if before the first subtitle
>> > + * 0 ~ i_subtitles-1 if falls into the time
>> > + * between the start of this sentence and the start of the next
>> (or
>> > the end time)
>> > + * @param b_between return whether the time is between two sentence.
>> > + * true if between the i_subtitle and the next one
>> > + * false if not between, but falls in the time of i_subtitle.
>> > + */
>> > +static void getSubtitleAtTime (int i_subtitles,
>> > + subtitle_t *subtitle,
>> > + int64_t i_time,
>> > + int * i_subtitle,
>> > + bool * b_between);
>> > +
>> >
>> /**************************************************************************
>> > *** * Module initializer
>> >
>> >
>> ***************************************************************************
>> > **/ @@ -590,6 +609,74 @@ static void Close( vlc_object_t *p_this )
>> > free( p_sys );
>> > }
>> >
>> > +/*
>> > + * Get the subtitle sentence at a specific time
>> > + * @param i_subtitles total number of subtitles regarding to *subtitle
>> > + * @param *subtitle pointer of array of the subtitle data
>> > + * @param i_time the specific time (in microsecond)
>> > + * @param i_subtitle return the number of subtitle sentence
>> > + * -1 if before the first subtitle
>> > + * 0 ~ i_subtitles-1 if falls into the time
>> > + * between the start of this sentence and the start of the next
>> (or
>> > the end time)
>> > + * @param b_between return whether the time is between two sentence.
>> > + * true if between the i_subtitle and the next one
>> > + * false if not between, but falls in the time of i_subtitle.
>> > + */
>> > +static void getSubtitleAtTime (int i_subtitles,
>> > + subtitle_t *subtitle,
>> > + int64_t i_time,
>> > + int * i_subtitle,
>> > + bool * b_between)
>> > +{
>> > + if (!subtitle || i_subtitles<=0)
>> > + {
>> > + *i_subtitle = -1; *b_between = false;
>> > + return;
>> > + }
>> > +
>> > + // binary search
>> > + int ll=0, rr=i_subtitles-1;
>> > +
>> > + while (ll<=rr)
>> > + {
>> > + int mm= (ll+rr) / 2;
>> > +
>> > + if ( i_time < subtitle[mm].i_start)
>> > + {
>> > + // if the timestamp is before the start of the middle node
>> > + // continue the search in the lower half
>> > + rr = mm-1;
>> > + } else if ( mm<rr && i_time >= subtitle[mm+1].i_start)
>> > + {
>> > + // if the middle node is not the right node, and the
>> timestamp
>> > + // is after the start of the node next to the middle
>> > + // continue the search in the higher half
>> > + ll = mm+1;
>> > +
>> > + } else
>> > + {
>> > + // otherwise the timestamp is between
>> > + // the start of #mm and the start of #mm+1
>> > + *i_subtitle = mm;
>> > + *b_between = i_time > subtitle[mm].i_stop;
>> > + return;
>> > + }
>> > + }
>> > +
>> > + if (rr<0)
>> > + {
>> > + // timestamp before the first subtitle sentence
>> > + *i_subtitle = -1; *b_between = true;
>> > + }
>> > + else
>> > + {
>> > + //should not be here
>> > + *i_subtitle = -1; *b_between = true;
>> > + }
>>
>> Did you try to use the built-in bsearch()?
>>
>> > +}
>> > +
>> > +
>> > +
>> >
>> /**************************************************************************
>> > *** * Control:
>> >
>> >
>> ***************************************************************************
>> > **/ @@ -678,6 +765,78 @@ static int Control( demux_t *p_demux, int
>> i_query,
>> > va_list args )
>> > case DEMUX_CAN_RECORD:
>> > return VLC_EGENERIC;
>> >
>> > + case DEMUX_IS_SUBTITLE_SEEKABLE:
>> > + {
>> > + bool *b = (bool*) va_arg (args, bool*);
>> > + *b = true;
>> > + return VLC_SUCCESS;
>> > + }
>> > + case DEMUX_GET_SUBTITLE_CUR_SENTENCE_TIME:
>> > + {
>> > + int64_t* pstart = (int64_t*)va_arg( args, int64_t * );
>> > + int64_t* pend = (int64_t*)va_arg( args, int64_t * );
>> > +
>> > + bool b;
>> > +
>> > + int64_t i_time = var_GetTime( p_demux->p_input, "time" );
>> > + int i_subtitle;
>>
>> In principles, p_demux->p_input can be NULL, so you should not blindly
>> assume
>> you can call var_GetTime() here. More generally, I may sound like a broken
>> record, but it is not advisable to manipulate the input from the demuxer,
>> not
>> even as read-only.
>> If nothing else, you could probably call var_GetTime() in input.c and
>> pass the
>> result as a parameter.
>>
>> That said, if at all possible, I think patching the ES output is even
>> better.
>>
>> > +
>> > + getSubtitleAtTime (p_sys->i_subtitles,
>> > + p_sys->subtitle, i_time, &i_subtitle, &b);
>> > + *pstart = p_sys->subtitle[i_subtitle].i_start;
>> > + *pend = p_sys->subtitle[i_subtitle].i_stop;
>> > +
>> > + return VLC_SUCCESS;
>> > + }
>> > +
>> > + case DEMUX_GET_SUBTITLE_NEXT_SENTENCE_TIME:
>> > + {
>> > + int64_t* pstart = (int64_t*)va_arg( args, int64_t * );
>> > + int64_t* pend = (int64_t*)va_arg( args, int64_t * );
>> > +
>> > + bool b;
>> > +
>> > + int64_t i_time = var_GetTime( p_demux->p_input, "time" );
>>
>> Ditto.
>>
>> > + int i_subtitle;
>> > +
>> > + getSubtitleAtTime (p_sys->i_subtitles,
>> > + p_sys->subtitle, i_time, &i_subtitle, &b);
>> > +
>> > + if (i_subtitle < p_sys->i_subtitles - 1)
>> > + i_subtitle ++;
>> > +
>> > + *pstart = p_sys->subtitle[i_subtitle].i_start;
>> > + *pend = p_sys->subtitle[i_subtitle].i_stop;
>> > +
>> > + return VLC_SUCCESS;
>> > + }
>> > +
>> > + case DEMUX_GET_SUBTITLE_PREV_SENTENCE_TIME:
>> > + {
>> > + int64_t* pstart = (int64_t*)va_arg( args, int64_t * );
>> > + int64_t* pend = (int64_t*)va_arg( args, int64_t * );
>> > +
>> > + bool b;
>> > +
>> > + int64_t i_time = var_GetTime( p_demux->p_input, "time" );
>>
>> Ditto.
>>
>> > + int i_subtitle;
>> > +
>> > + getSubtitleAtTime (p_sys->i_subtitles,
>> > + p_sys->subtitle, i_time, &i_subtitle, &b);
>> > +
>> > + /* If currently between two sentence (b=true),
>> > + * just jump to the "current" sentence before the break;
>> > + * Otherwise
>> > + * jump to the sentence before
>> > + */
>> > + if (i_subtitle > 0 && !b)
>> > + i_subtitle --;
>> > +
>> > + *pstart = p_sys->subtitle[i_subtitle].i_start;
>> > + *pend = p_sys->subtitle[i_subtitle].i_stop;
>> > +
>> > + return VLC_SUCCESS;
>> > + }
>> > default:
>> > msg_Err( p_demux, "unknown query %d in subtitle control",
>> > i_query );
>> > return VLC_EGENERIC;
>> > diff --git a/src/input/control.c b/src/input/control.c
>> > index 2e4633f..c8296a0 100644
>> > --- a/src/input/control.c
>> > +++ b/src/input/control.c
>> > @@ -35,6 +35,7 @@
>> > #include "event.h"
>> > #include "resource.h"
>> > #include "es_out.h"
>> > +#include "demux.h"
>> >
>> >
>> > static void UpdateBookmarksOption( input_thread_t * );
>> > @@ -494,6 +495,40 @@ int input_vaControl( input_thread_t *p_input, int
>> > i_query, va_list args )
>> > return es_out_ControlModifyPcrSystem( p_input->p-
>> >
>> > >p_es_out_display, b_absolute, i_system );
>> >
>> > }
>> >
>> > + case INPUT_GET_CUR_SENTENCE_TIME:
>> > + case INPUT_GET_NEXT_SENTENCE_TIME:
>> > + case INPUT_GET_PREV_SENTENCE_TIME:
>> > + {
>> > + input_source_t * p_sub = p_input->p->p_def_sub;
>>
>> p_def_sub is written in the input thread. You cannot blindly read it from
>> another thread.
>>
>> > +
>> > + if (!p_sub)
>> > + return VLC_EGENERIC;
>> > +
>> > + int64_t* pstart = (int64_t*)va_arg( args, int64_t * );
>> > + int64_t* pend = (int64_t*)va_arg( args, int64_t * );
>> > +
>> > + if (!demux_Control (p_sub->p_demux ,
>> > + DEMUX_GET_SUBTITLE_CUR_SENTENCE_TIME +
>> > + i_query - INPUT_GET_CUR_SENTENCE_TIME,
>> > + pstart, pend))
>> > + return VLC_SUCCESS;
>> > + else
>> > + return VLC_EGENERIC;
>>
>> This assumes that the subtitle demux is re-entrant - which it is not.
>>
>> Compare INPUT_ADD_SUBTITLE / INPUT_CONTROL_ADD_SUBTITLE,
>>
>> > + }
>> > + case INPUT_GET_SUBTITLE_SEEKABLE:
>> > + {
>> > + bool * p_seekable = (bool*)va_arg( args, bool * );
>> > + input_source_t * p_sub = p_input->p->p_def_sub;
>> > +
>> > + *p_seekable = false;
>> > +
>> > + if (p_sub)
>> > + demux_Control (p_sub->p_demux,
>> > + DEMUX_IS_SUBTITLE_SEEKABLE,
>> > + p_seekable);
>> > +
>> > + return VLC_SUCCESS;
>> > + }
>>
>> Ditto.
>>
>> > default:
>> > msg_Err( p_input, "unknown query in input_vaControl" );
>> > return VLC_EGENERIC;
>> > diff --git a/src/input/event.c b/src/input/event.c
>> > index e32ca8a..f887021 100644
>> > --- a/src/input/event.c
>> > +++ b/src/input/event.c
>> > @@ -191,6 +191,13 @@ void input_SendEventCache( input_thread_t *p_input,
>> > double f_level )
>> > Trigger( p_input, INPUT_EVENT_CACHE );
>> > }
>> >
>> > +/* Send a signal notifying a default subtitle loaded */
>> > +void input_SendEventDefSubLoaded( input_thread_t *p_input )
>> > +{
>> > + Trigger( p_input, INPUT_EVENT_DEFAULT_SUBTITLE_LOADED);
>> > +}
>> > +
>> > +
>> > /* FIXME: review them because vlc_event_send might be
>> > * moved inside input_item* functions.
>> > */
>> > diff --git a/src/input/event.h b/src/input/event.h
>> > index e91ad11..4a63661 100644
>> > --- a/src/input/event.h
>> > +++ b/src/input/event.h
>> > @@ -42,6 +42,7 @@ void input_SendEventSeekpoint( input_thread_t
>> *p_input,
>> > int i_title, int i_seekp
>> > void input_SendEventSignal( input_thread_t *p_input, double f_quality,
>> > double f_strength );
>> > void input_SendEventState( input_thread_t *p_input, int i_state );
>> > void input_SendEventCache( input_thread_t *p_input, double f_level );
>> > +void input_SendEventDefSubLoaded( input_thread_t *p_input );
>> >
>> > /* TODO rename Item* */
>> > void input_SendEventMeta( input_thread_t *p_input );
>> > diff --git a/src/input/input.c b/src/input/input.c
>> > index 7de30ec..2c2a55a 100644
>> > --- a/src/input/input.c
>> > +++ b/src/input/input.c
>> > @@ -369,6 +369,9 @@ static input_thread_t *Create( vlc_object_t
>> *p_parent,
>> > input_item_t *p_item,
>> > /* No slave */
>> > p_input->p->i_slave = 0;
>> > p_input->p->slave = NULL;
>> > +
>> > + /* No default subtitle */
>> > + p_input->p->p_def_sub = NULL;
>> >
>> > /* */
>> > if( p_resource )
>> > @@ -2982,6 +2985,11 @@ static void input_SubtitleAdd( input_thread_t
>> > *p_input,
>> >
>> > es_out_Control( p_input->p->p_es_out_display,
>> > ES_OUT_SET_ES_DEFAULT_BY_ID, i_id );
>> > es_out_Control( p_input->p->p_es_out_display,
>> ES_OUT_SET_ES_BY_ID,
>> > i_id );
>> > +
>> > + /* set default subtitle input source and
>> > + * send the notification event*/
>> > + p_input->p->p_def_sub = sub;
>> > + input_SendEventDefSubLoaded(p_input);
>> > }
>> > var_FreeList( &list, NULL );
>> > }
>> > diff --git a/src/input/input_internal.h b/src/input/input_internal.h
>> > index fa8c2b1..98fca02 100644
>> > --- a/src/input/input_internal.h
>> > +++ b/src/input/input_internal.h
>> > @@ -129,6 +129,9 @@ struct input_thread_private_t
>> > int i_slave;
>> > input_source_t **slave;
>> >
>> > + /* Default subtitle input source */
>> > + input_source_t *p_def_sub;
>> > +
>> > /* Resources */
>> > input_resource_t *p_resource;
>> > input_resource_t *p_resource_private;
>>
>> --
>> Rémi Denis-Courmont
>> http://www.remlab.net/
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20150324/574debc5/attachment.html>
More information about the vlc-devel
mailing list