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

[Filip Roséen]

Given that the matroska format can contain hidden chapters (and that the
demuxer itself create such where it feels applicable), we should not expose
such chapters (as seekpoints) outside of the demuxer module.

This commit fixes the ticket linked below by checking whether or not the
chapter should be displayed to the user, and only appends the item to the
title's seekpoint array if this is the case.

    - https://trac.videolan.org/vlc/ticket/17202

refs #17202

---

I have tested this commit locally with manually edited files with
explicitly hidden chapters and everything is in order as far as I can
tell, though the relevant logic inside parts surrounding this commit is
rather complex and a clean-up is most definitely in order.

Even though the fix is trivial, I have marked this patch as RFC to get
confirmation from TypX, Robux4 (or someone else who have spent time with
the mkv-demuxer in the past) for comments.

A few questions:

    - What was the original purpose/difference between;
        - chapter_item_c::b_user_display, and;
        - chapter_item_c::b_display_seekpoint,
      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 )
+        {
+            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");
 
-        /* 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;
 
-- 
2.9.0