[vlc-devel] [PATCH] mkv: remove typeid code in EBML dispatcher
Alexandre Janniaux
ajanni at videolabs.io
Tue Jun 16 10:59:40 CEST 2020
Hi,
Thank you for review!
> > - 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.
It's indirectly related to the fix, because this macro is orginally :
#define MKV_IS_ID( el, C ) ( el != NULL && typeid( *el ) == typeid( C ) )
#define MKV_CHECKED_PTR_DECL( name, type, src ) type * name = MKV_IS_ID(src, type) ? static_cast<type*>(src) : NULL
So having type = KaxChapterProcessTime wasn't an isssue, typeid
would work correctly with or without the const.
However by removing typeinfo, I replaced MKV_IS_ID into
#define MKV_IS_ID( el, C ) ( el != NULL && (el->operator const EbmlId&()) == (C::ClassInfos.ClassId()) )
Which, with the code where MKV_CHECKED_PTR_DECL_CONST is used,
would evaluate into:
( el != NULL && (el->operator const EbmlId&()) == (KaxChapterProcessTime const::ClassInfos.ClassId()) )
which is of course invalid syntax.
I planned to potentially improve it later using modern C++ syntax
but in order to split the changes I just simplified it with a
variant of the original macro for const type variable declaration.
Is it ok for you?
Regards,
--
Alexandre Janniaux
Videolabs
On Tue, Jun 16, 2020 at 10:47:53AM +0200, Steve Lhomme wrote:
> 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
> >
> _______________________________________________
> 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