[vlc-devel] [PATCH] mkv: remove typeid code in EBML dispatcher

Alexandre Janniaux ajanni at videolabs.io
Mon Jun 15 16:30:53 CEST 2020


EBML can associate multiple class to a single EBML ID, which mean that
it potentially needs typeid checks. However, libmatroska always exposes
a single type per EBML ID, so it never needs those checks.

In addition, those checks are leading to warnings (attached below) and
issues depending on the visibility and optimization level on clang. See
the following mail on the mailing list for reference:

https://lists.llvm.org/pipermail/llvm-dev/2014-June/073465.html

To sum up, typeinfo are becoming different between libmatroska and the
matroska modules for the same classes, so the matroska demuxer is never
able to open correctly in those case, and it fallbacks on avformat
demuxer if available.

Compilation warning fixed by this patch:

    ../../../modules/demux/mkv/chapter_command.cpp:35:13: warning: expression with side effects will be evaluated despite being used as an operand to 'typeid'
          [-Wpotentially-evaluated-expression]
            if( MKV_CHECKED_PTR_DECL( p_cpt, KaxChapterProcessTime const, command[i] ) )
                ^
    ../../../modules/demux/mkv/mkv.hpp:116:63: note: expanded from macro 'MKV_CHECKED_PTR_DECL'
    #define MKV_CHECKED_PTR_DECL( name, type, src ) type * name = MKV_IS_ID(src, type) ? static_cast<type*>(src) : NULL
                                                                  ^
    ../../../modules/demux/mkv/mkv.hpp:115:52: note: expanded from macro 'MKV_IS_ID'
    #define MKV_IS_ID( el, C ) ( el != NULL && typeid( *el ) == typeid( C ) )
                                                       ^
    ../../../modules/demux/mkv/chapter_command.cpp:44:13: warning: expression with side effects will be evaluated despite being used as an operand to 'typeid'
          [-Wpotentially-evaluated-expression]
            if( MKV_CHECKED_PTR_DECL( p_cpd, KaxChapterProcessData const, command[i] ) )
                ^
    ../../../modules/demux/mkv/mkv.hpp:116:63: note: expanded from macro 'MKV_CHECKED_PTR_DECL'
    #define MKV_CHECKED_PTR_DECL( name, type, src ) type * name = MKV_IS_ID(src, type) ? static_cast<type*>(src) : NULL
                                                                  ^
    ../../../modules/demux/mkv/mkv.hpp:115:52: note: expanded from macro 'MKV_IS_ID'
    #define MKV_IS_ID( el, C ) ( el != NULL && typeid( *el ) == typeid( C ) )

Fix videolan/VLCKit#372
---
 modules/demux/mkv/Ebml_dispatcher.hpp | 49 ++++-----------------------
 modules/demux/mkv/chapter_command.cpp |  4 +--
 modules/demux/mkv/mkv.hpp             |  3 +-
 3 files changed, 11 insertions(+), 45 deletions(-)

diff --git a/modules/demux/mkv/Ebml_dispatcher.hpp b/modules/demux/mkv/Ebml_dispatcher.hpp
index 85bf03e6094..8d1e7a60538 100644
--- a/modules/demux/mkv/Ebml_dispatcher.hpp
+++ b/modules/demux/mkv/Ebml_dispatcher.hpp
@@ -34,23 +34,14 @@
 #include <vector>
 
 namespace {
-  namespace detail {
-    template<class T>
-    static std::type_info const* typeid_ptr () {
-      static std::type_info const& ti = typeid (T);
-      return &ti;
-    }
-  }
-
   struct EbmlProcessorEntry {
     typedef void (*EbmlProcessor) (EbmlElement*, void*);
 
     EbmlId         const* p_ebmlid;
-    std::type_info const* p_typeid;
     EbmlProcessor         callback;
 
-    EbmlProcessorEntry (EbmlId const& id, std::type_info const* ti, EbmlProcessor cb)
-      : p_ebmlid (&id), p_typeid (ti), callback (cb)
+    EbmlProcessorEntry (EbmlId const& id, EbmlProcessor cb)
+      : p_ebmlid (&id), callback (cb)
     { }
 
   };
@@ -84,7 +75,7 @@ namespace {
             return false;
 
         EbmlProcessorEntry eb = EbmlProcessorEntry (
-          static_cast<EbmlId const&> (*element), NULL, NULL
+          static_cast<EbmlId const&> (*element), NULL
         );
 
         // --------------------------------------------------------------
@@ -98,26 +89,9 @@ namespace {
 
         if (cit != cit_end)
         {
-          // --------------------------------------------------------------
-          // normally we only need to compare the addresses of the EbmlId
-          // since libebml returns a reference to a _static_ instance.
-          // --------------------------------------------------------------
-
-          while (cit != cit_end && (cit->p_ebmlid == eb.p_ebmlid || (*cit->p_ebmlid == *eb.p_ebmlid))) {
-            std::type_info const& ti = typeid (*element);
-
-            // --------------------------------------------------------------
-            // even though the EbmlId are equivalent, we still need to make
-            // sure that the typeid also matches.
-            // --------------------------------------------------------------
-
-            if (*(cit->p_typeid) == ti) {
-              cit->callback (element, payload);
-              return true;
-            }
-
-            ++cit;
-          }
+            assert(cit->p_ebmlid == eb.p_ebmlid || (*cit->p_ebmlid == *eb.p_ebmlid));
+            cit->callback (element, payload);
+            return true;
         }
 
         if (_default_handler == NULL)
@@ -138,18 +112,9 @@ namespace {
       InitializationExpr_, static_cast<EbmlType_&> (*data)                            \
     )
 
-// -----------------------------------------------------------------------------------
-// The use of `detail::typeid_ptr` below is so that we do not have to invoke "typeid"
-// every time we are requested to do a lookup. `std::type_info` cannot be copied, so
-// we cannot pass it by value.
-//
-// In C++11 you could use the hash value present inside std::type_info to do lookup,
-// but we are stuck in C++03 and have to use the below instead.
-// -----------------------------------------------------------------------------------
-
 #define E_CASE(EbmlType_, VariableName_)            \
     EBML_ELEMENT_CASE_DEF(EbmlType_, EbmlType_, VariableName_, \
-      (dispatcher.insert( EbmlProcessorEntry( EbmlType_ ::ClassInfos.ClassId(), detail::typeid_ptr<EbmlType_>(), &EbmlType_ ## _callback) ) ) \
+      (dispatcher.insert( EbmlProcessorEntry( EbmlType_ ::ClassInfos.ClassId(), &EbmlType_ ## _callback) ) ) \
     )
 
 #define E_CASE_DEFAULT(VariableName_)                    \
diff --git a/modules/demux/mkv/chapter_command.cpp b/modules/demux/mkv/chapter_command.cpp
index 9a8749b8795..9a23b77dfec 100644
--- a/modules/demux/mkv/chapter_command.cpp
+++ b/modules/demux/mkv/chapter_command.cpp
@@ -32,7 +32,7 @@ void chapter_codec_cmds_c::AddCommand( const KaxChapterProcessCommand & command
     uint32 codec_time = uint32(-1);
     for( size_t i = 0; i < command.ListSize(); i++ )
     {
-        if( MKV_CHECKED_PTR_DECL( p_cpt, KaxChapterProcessTime const, command[i] ) )
+        if( MKV_CHECKED_PTR_DECL_CONST( p_cpt, KaxChapterProcessTime, command[i] ) )
         {
             codec_time = static_cast<uint32>( *p_cpt );
             break;
@@ -41,7 +41,7 @@ void chapter_codec_cmds_c::AddCommand( const KaxChapterProcessCommand & command
 
     for( size_t i = 0; i < command.ListSize(); i++ )
     {
-        if( MKV_CHECKED_PTR_DECL( p_cpd, KaxChapterProcessData const, command[i] ) )
+        if( MKV_CHECKED_PTR_DECL_CONST( p_cpd, KaxChapterProcessData, command[i] ) )
         {
             std::vector<KaxChapterProcessData*> *containers[] = {
                 &during_cmds, /* codec_time = 0 */
diff --git a/modules/demux/mkv/mkv.hpp b/modules/demux/mkv/mkv.hpp
index 1e9a40e510e..a061254228f 100644
--- a/modules/demux/mkv/mkv.hpp
+++ b/modules/demux/mkv/mkv.hpp
@@ -112,8 +112,9 @@ enum
 
 #define MKVD_TIMECODESCALE 1000000
 
-#define MKV_IS_ID( el, C ) ( el != NULL && typeid( *el ) == typeid( C ) )
+#define MKV_IS_ID( el, C ) ( el != NULL && (el->operator const EbmlId&()) == (C::ClassInfos.ClassId()) )
 #define MKV_CHECKED_PTR_DECL( name, type, src ) type * name = MKV_IS_ID(src, type) ? static_cast<type*>(src) : NULL
+#define MKV_CHECKED_PTR_DECL_CONST( name, type, src ) const type * name = MKV_IS_ID(src, type) ? static_cast<const type*>(src) : NULL
 
 
 using namespace LIBMATROSKA_NAMESPACE;
-- 
2.27.0



More information about the vlc-devel mailing list