[vlc-devel] [RFC PATCH] demux/mkv: do not expose hidden chapters

[Denis Charmet]

Hi,

On 2016-07-24 00:56, Filip Roséen wrote:
> A few questions:
> 
>     - What was the original purpose/difference between;
>         - chapter_item_c::b_user_display, and;

It means that the chapter has at least one Chapter Display string so we 
have actually something to display as a name in the seekpoint

>         - chapter_item_c::b_display_seekpoint,

It means that the chapter isn't hidden (or that there is a 
chapter_command). I cannot tell for chapter commands...

>       other than that one defaults to false, and the other to true?
> 
>     - Can this patch be pushed as is (prior to a clean-up of relevant
>       sections around it)?
> 
> Thank you!
> ---
>  modules/demux/mkv/virtual_segment.cpp | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/modules/demux/mkv/virtual_segment.cpp
> b/modules/demux/mkv/virtual_segment.cpp
> index 5d30d7e..2bce464 100644
> --- a/modules/demux/mkv/virtual_segment.cpp
> +++ b/modules/demux/mkv/virtual_segment.cpp
> @@ -592,22 +592,24 @@ int virtual_chapter_c::PublishChapters(
> input_title_t & title, int & i_user_chap
>           ( ( sub_vchapters.size() > 0 && i_mk_virtual_start_time !=
> sub_vchapters[0]->i_mk_virtual_start_time) ||
>             sub_vchapters.size() == 0 ) ) || !p_chapter )
>      {
> -        seekpoint_t *sk = vlc_seekpoint_New();
> +        if( p_chapter && p_chapter->b_user_display )

While I don't mind the fact that you don't publish dummy chapters (and 
yes I should have removed that a long time ago) I'd rather have you 
remove the "|| !p_chapter" above.

Besides since I don't know how chapter commands work this might break 
chapters which have a codecName infered from chapter commands but no 
Chapter display string.

> +        {
> +            seekpoint_t *sk = vlc_seekpoint_New();
> 
> -        sk->i_time_offset = i_mk_virtual_start_time;
> -        if( p_chapter )
> -            sk->psz_name = strdup( p_chapter->psz_name.c_str() );
> -        else
> -            sk->psz_name = strdup("dummy chapter");
> +            sk->i_time_offset = i_mk_virtual_start_time;
> +            if( p_chapter )
> +                sk->psz_name = strdup( p_chapter->psz_name.c_str() );
> +            else
> +                sk->psz_name = strdup("dummy chapter");

remove that it will never happen

> 
> -        /* A start time of '0' is ok. A missing ChapterTime element
> is ok, too, because '0' is its default value. */
> -        title.i_seekpoint++;
> -        title.seekpoint = (seekpoint_t**)xrealloc( title.seekpoint,
> -                                 title.i_seekpoint * sizeof( 
> seekpoint_t* ) );
> -        title.seekpoint[title.i_seekpoint-1] = sk;
> +            /* A start time of '0' is ok. A missing ChapterTime
> element is ok, too, because '0' is its default value. */
> +            title.i_seekpoint++;
> +            title.seekpoint = (seekpoint_t**)xrealloc( 
> title.seekpoint,
> +              title.i_seekpoint * sizeof( seekpoint_t* ) );
> +            title.seekpoint[title.i_seekpoint-1] = sk;
> 
> -        if ( (p_chapter && p_chapter->b_user_display ) ||  !p_chapter 
> )
>              i_user_chapters++;
> +        }
>      }
>      i_seekpoint_num = i_user_chapters;

Regards,
-- 
Denis Charmet - TypX
Le mauvais esprit est un art de vivre