[vlc-devel] [PATCH] mkv: remove typeid code in EBML dispatcher
Steve Lhomme
robux4 at ycbcr.xyz
Tue Jun 16 10:47:53 CEST 2020
Hi,
LGTM with the comment below.
On 2020-06-15 16:30, Alexandre Janniaux wrote:
> 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] ) )
Is this related to the fix or just some extra warning cleaning ? If it's
just cleaning it would be better in a different patch.
> {
> 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
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>
More information about the vlc-devel
mailing list