[vlc-devel] [PATCH 16/20] mkv: removed manual memory management from Tags + SimpleTags

Filip Roséen filip at videolabs.io
Fri Mar 4 17:04:14 CET 2016


In order to provide better runtime safety manual management of dynamic
memory has been removed from `class Tags` and `class SimpleTags`.

This includes introducing usage of `std::vector` as well as
`std::string`, while also removing data-members that are no longer
required.

 - SimpleTag::b_default has been removed since it was written to (at
   once place), but never read; the write has been replaced by a comment.

 - The ParseSimpleTag function has been changed to return `bool` instead
   of a pointer to signal whether parsing was successful. The result
   will be written to `pout_simple` (instead of being indirectly
   returned through a pointer).
---
 modules/demux/mkv/demux.cpp            |  14 ++--
 modules/demux/mkv/matroska_segment.cpp | 117 +++++++++++++--------------------
 modules/demux/mkv/matroska_segment.hpp |  24 ++++---
 3 files changed, 62 insertions(+), 93 deletions(-)

diff --git a/modules/demux/mkv/demux.cpp b/modules/demux/mkv/demux.cpp
index 2fbc69a..f2eed13 100644
--- a/modules/demux/mkv/demux.cpp
+++ b/modules/demux/mkv/demux.cpp
@@ -682,21 +682,21 @@ bool demux_sys_t::PreloadLinked()
                         /* Check in tags if the edition has a name */
 
                         /* We use only the tags of the first segment as it contains the edition */
-                        std::vector<Tag*> &tags = opened_segments[0]->tags;
+                        matroska_segment_c::tags_t const& tags = opened_segments[0]->tags;
                         uint64_t i_ed_uid = 0;
                         if( p_ved->p_edition )
                             i_ed_uid = (uint64_t) p_ved->p_edition->i_uid;
 
                         for( size_t k = 0; k < tags.size(); k++ )
                         {
-                            if( tags[k]->i_tag_type == EDITION_UID && tags[k]->i_uid == i_ed_uid )
-                                for( size_t l = 0; l < tags[k]->simple_tags.size(); l++ )
+                            if( tags[k].i_tag_type == EDITION_UID && tags[k].i_uid == i_ed_uid )
+                                for( size_t l = 0; l < tags[k].simple_tags.size(); l++ )
                                 {
-                                    SimpleTag * p_st = tags[k]->simple_tags[l];
-                                    if( !strcmp(p_st->psz_tag_name,"TITLE") )
+                                    SimpleTag const& st = tags[k].simple_tags[l];
+                                    if ( st.tag_name == "TITLE" )
                                     {
-                                        msg_Dbg( &demuxer, "Using title \"%s\" from tag for edition %" PRIu64, p_st->p_value, i_ed_uid );
-                                        p_title->psz_name = strdup( p_st->p_value );
+                                        msg_Dbg( &demuxer, "Using title \"%s\" from tag for edition %" PRIu64, st.value.c_str (), i_ed_uid );
+                                        p_title->psz_name = strdup( st.value.c_str () );
                                         break;
                                     }
                                 }
diff --git a/modules/demux/mkv/matroska_segment.cpp b/modules/demux/mkv/matroska_segment.cpp
index 693d68e..b3947b3 100644
--- a/modules/demux/mkv/matroska_segment.cpp
+++ b/modules/demux/mkv/matroska_segment.cpp
@@ -42,7 +42,6 @@ matroska_segment_c::matroska_segment_c( demux_sys_t & demuxer, EbmlStream & estr
     ,i_tracks_position(-1)
     ,i_info_position(-1)
     ,i_chapters_position(-1)
-    ,i_tags_position(-1)
     ,i_attachments_position(-1)
     ,cluster(NULL)
     ,i_block_pos(0)
@@ -94,7 +93,6 @@ matroska_segment_c::~matroska_segment_c()
     vlc_delete_all( stored_editions );
     vlc_delete_all( translations );
     vlc_delete_all( families );
-    vlc_delete_all( tags );
 }
 
 
@@ -257,20 +255,13 @@ static const struct {
                      {vlc_meta_Title,       NULL,            0},
 };
 
-SimpleTag * matroska_segment_c::ParseSimpleTags( KaxTagSimple *tag, int target_type )
+bool matroska_segment_c::ParseSimpleTags( SimpleTag* pout_simple, KaxTagSimple *tag, int target_type )
 {
     EbmlParser eparser ( &es, tag, &sys.demuxer, var_InheritBool( &sys.demuxer, "mkv-use-dummy" ) );
     EbmlElement *el;
-    SimpleTag * p_simple = new (std::nothrow) SimpleTag;
     size_t max_size = tag->GetSize();
     size_t size = 0;
 
-    if( !p_simple )
-    {
-        msg_Err( &sys.demuxer, "Couldn't allocate memory for Simple Tag... ignoring it");
-        return NULL;
-    }
-
     if( !sys.meta )
         sys.meta = vlc_meta_New();
 
@@ -283,35 +274,36 @@ SimpleTag * matroska_segment_c::ParseSimpleTags( KaxTagSimple *tag, int target_t
             {
                 msg_Err( &sys.demuxer, "Error %s too big ignoring the tag", typeid(*el).name() );
                 delete ep;
-                delete p_simple;
-                return NULL;
+                return false;
             }
             if( MKV_CHECKED_PTR_DECL ( ktn_ptr, KaxTagName, el ) )
             {
                 ktn_ptr->ReadData( es.I_O(), SCOPE_ALL_DATA );
-                p_simple->psz_tag_name = strdup( UTFstring( *ktn_ptr ).GetUTF8().c_str() );
+                pout_simple->tag_name = UTFstring( *ktn_ptr ).GetUTF8().c_str();
             }
             else if( MKV_CHECKED_PTR_DECL ( kts_ptr, KaxTagString, el ) )
             {
                 kts_ptr->ReadData( es.I_O(), SCOPE_ALL_DATA );
-                p_simple->p_value = strdup( UTFstring( *kts_ptr ).GetUTF8().c_str() );
+                pout_simple->value = UTFstring( *kts_ptr ).GetUTF8().c_str();
             }
             else if(  MKV_CHECKED_PTR_DECL ( ktl_ptr, KaxTagLangue, el ) )
             {
                 ktl_ptr->ReadData( es.I_O(), SCOPE_ALL_DATA );
-                p_simple->psz_lang = strdup( std::string( *ktl_ptr ).c_str());
+                pout_simple->lang = *ktl_ptr;
             }
             else if(  MKV_CHECKED_PTR_DECL ( ktd_ptr, KaxTagDefault, el ) )
             {
-                ktd_ptr->ReadData( es.I_O(), SCOPE_ALL_DATA );
-                p_simple->b_default = (bool) uint8( *ktd_ptr );
+                VLC_UNUSED(ktd_ptr); // TODO: we do not care about this value, but maybe we should?
             }
             /*Tags can be nested*/
             else if( MKV_CHECKED_PTR_DECL ( kts_ptr, KaxTagSimple, el) )
             {
-                SimpleTag * p_st = ParseSimpleTags( kts_ptr, target_type );
-                if( p_st )
-                    p_simple->sub_tags.push_back( p_st );
+                SimpleTag st; // ParseSimpleTags will write to this variable
+                              // the SimpleTag is valid if ParseSimpleTags returns `true`
+
+                if (ParseSimpleTags( &st, kts_ptr, target_type )) {
+                  pout_simple->sub_tags.push_back( st );
+                }
             }
             /*TODO Handle binary tags*/
             size += el->HeadSize() + el->GetSize();
@@ -321,30 +313,28 @@ SimpleTag * matroska_segment_c::ParseSimpleTags( KaxTagSimple *tag, int target_t
     {
         msg_Err( &sys.demuxer, "Error while reading Tag ");
         delete ep;
-        delete p_simple;
-        return NULL;
+        return false;
     }
 
-    if( !p_simple->psz_tag_name || !p_simple->p_value )
+    if( pout_simple->tag_name.empty() )
     {
         msg_Warn( &sys.demuxer, "Invalid MKV SimpleTag found.");
-        delete p_simple;
-        return NULL;
+        return false;
     }
     for( int i = 0; metadata_map[i].key; i++ )
     {
-        if( !strcmp( p_simple->psz_tag_name, metadata_map[i].key ) &&
+        if( pout_simple->tag_name == metadata_map[i].key &&
             (metadata_map[i].target_type == 0 || target_type == metadata_map[i].target_type ) )
         {
-            vlc_meta_Set( sys.meta, metadata_map[i].type, p_simple->p_value );
-            msg_Dbg( &sys.demuxer, "|   |   + Meta %s: %s", p_simple->psz_tag_name, p_simple->p_value);
+            vlc_meta_Set( sys.meta, metadata_map[i].type, pout_simple->value.c_str () );
+            msg_Dbg( &sys.demuxer, "|   |   + Meta %s: %s", pout_simple->tag_name.c_str (), pout_simple->value.c_str ());
             goto done;
         }
     }
-    msg_Dbg( &sys.demuxer, "|   |   + Meta %s: %s", p_simple->psz_tag_name, p_simple->p_value);
-    vlc_meta_AddExtra( sys.meta, p_simple->psz_tag_name, p_simple->p_value);
+    msg_Dbg( &sys.demuxer, "|   |   + Meta %s: %s", pout_simple->tag_name.c_str (), pout_simple->value.c_str ());
+    vlc_meta_AddExtra( sys.meta, pout_simple->tag_name.c_str (), pout_simple->value.c_str ());
 done:
-    return p_simple;
+    return true;
 }
 
 #define PARSE_TAG( type ) \
@@ -368,12 +358,8 @@ void matroska_segment_c::LoadTags( KaxTags *tags )
     {
         if( MKV_IS_ID( el, KaxTag ) )
         {
-            Tag * p_tag = new (std::nothrow) Tag;
-            if(!p_tag)
-            {
-                msg_Err( &sys.demuxer,"Couldn't allocate memory for tag... ignoring it");
-                continue;
-            }
+            Tag tag;
+
             msg_Dbg( &sys.demuxer, "+ Tag" );
             eparser.Down();
             int target_type = 50;
@@ -401,32 +387,32 @@ void matroska_segment_c::LoadTags( KaxTags *tags )
                             }
                             else if( MKV_CHECKED_PTR_DECL ( kttu_ptr, KaxTagTrackUID, el ) )
                             {
-                                p_tag->i_tag_type = TRACK_UID;
+                                tag.i_tag_type = TRACK_UID;
                                 kttu_ptr->ReadData( es.I_O() );
-                                p_tag->i_uid = static_cast<uint64>( *kttu_ptr );
-                                msg_Dbg( &sys.demuxer, "|   |   + TrackUID: %" PRIu64, p_tag->i_uid);
+                                tag.i_uid = static_cast<uint64>( *kttu_ptr );
+                                msg_Dbg( &sys.demuxer, "|   |   + TrackUID: %" PRIu64, tag.i_uid);
 
                             }
                             else if( MKV_CHECKED_PTR_DECL ( kteu_ptr, KaxTagEditionUID, el ) )
                             {
-                                p_tag->i_tag_type = EDITION_UID;
+                                tag.i_tag_type = EDITION_UID;
                                 kteu_ptr->ReadData( es.I_O() );
-                                p_tag->i_uid = static_cast<uint64>( *kteu_ptr );
-                                msg_Dbg( &sys.demuxer, "|   |   + EditionUID: %" PRIu64, p_tag->i_uid);
+                                tag.i_uid = static_cast<uint64>( *kteu_ptr );
+                                msg_Dbg( &sys.demuxer, "|   |   + EditionUID: %" PRIu64, tag.i_uid);
                             }
                             else if( MKV_CHECKED_PTR_DECL ( ktcu_ptr, KaxTagChapterUID, el ) )
                             {
-                                p_tag->i_tag_type = CHAPTER_UID;
+                                tag.i_tag_type = CHAPTER_UID;
                                 ktcu_ptr->ReadData( es.I_O() );
-                                p_tag->i_uid = static_cast<uint64>( *ktcu_ptr );
-                                msg_Dbg( &sys.demuxer, "|   |   + ChapterUID: %" PRIu64, p_tag->i_uid);
+                                tag.i_uid = static_cast<uint64>( *ktcu_ptr );
+                                msg_Dbg( &sys.demuxer, "|   |   + ChapterUID: %" PRIu64, tag.i_uid);
                             }
                             else if( MKV_CHECKED_PTR_DECL ( ktau_ptr, KaxTagAttachmentUID, el ) )
                             {
-                                p_tag->i_tag_type = ATTACHMENT_UID;
+                                tag.i_tag_type = ATTACHMENT_UID;
                                 ktau_ptr->ReadData( es.I_O() );
-                                p_tag->i_uid = static_cast<uint64>( *ktau_ptr );
-                                msg_Dbg( &sys.demuxer, "|   |   + AttachmentUID: %" PRIu64, p_tag->i_uid);
+                                tag.i_uid = static_cast<uint64>( *ktau_ptr );
+                                msg_Dbg( &sys.demuxer, "|   |   + AttachmentUID: %" PRIu64, tag.i_uid);
                             }
                             else
                             {
@@ -443,9 +429,11 @@ void matroska_segment_c::LoadTags( KaxTags *tags )
                 }
                 else if( MKV_CHECKED_PTR_DECL ( kts_ptr, KaxTagSimple, el ) )
                 {
-                    SimpleTag * p_simple = ParseSimpleTags( kts_ptr, target_type );
-                    if( p_simple )
-                        p_tag->simple_tags.push_back( p_simple );
+                    SimpleTag simple;
+
+                    if (ParseSimpleTags(&simple, kts_ptr, target_type )) {
+                      tag.simple_tags.push_back( simple );
+                    }
                 }
 #if 0 // not valid anymore
                 else if( MKV_IS_ID( el, KaxTagGeneral ) )
@@ -491,7 +479,7 @@ void matroska_segment_c::LoadTags( KaxTags *tags )
                 }
             }
             eparser.Up();
-            this->tags.push_back(p_tag);
+            this->tags.push_back(tag);
         }
         else
         {
@@ -542,7 +530,7 @@ void matroska_segment_c::InformationCreate( )
     }
 #endif
 #if 0
-    if( i_tags_position >= 0 )
+    if( !tags.empty () )
     {
         bool b_seekable;
 
@@ -707,10 +695,9 @@ bool matroska_segment_c::Preload( )
         else if( MKV_CHECKED_PTR_DECL ( kt_ptr, KaxTags, el ) )
         {
             msg_Dbg( &sys.demuxer, "|   + Tags" );
-            if( i_tags_position < 0)
+            if(tags.empty ())
             {
                 LoadTags( kt_ptr );
-                i_tags_position = el->GetElementPosition();
             }
         }
         else if( MKV_IS_ID ( el, EbmlVoid ) )
@@ -812,10 +799,9 @@ bool matroska_segment_c::LoadSeekHeadItem( const EbmlCallbacks & ClassInfos, int
     else if( MKV_CHECKED_PTR_DECL ( kt_ptr, KaxTags, el ) )
     {
         msg_Dbg( &sys.demuxer, "|   + Tags" );
-        if( i_tags_position < 0 )
+        if(tags.empty ())
         {
             LoadTags( kt_ptr );
-            i_tags_position = i_element_position;
         }
     }
     else
@@ -1557,18 +1543,3 @@ int matroska_segment_c::BlockGet( KaxBlock * & pp_block, KaxSimpleBlock * & pp_s
         }
     }
 }
-
-SimpleTag::~SimpleTag()
-{
-    free(psz_tag_name);
-    free(psz_lang);
-    free(p_value);
-    for(size_t i = 0; i < sub_tags.size(); i++)
-        delete sub_tags[i];
-}
-
-Tag::~Tag()
-{
-    for(size_t i = 0; i < simple_tags.size(); i++)
-        delete simple_tags[i];
-}
diff --git a/modules/demux/mkv/matroska_segment.hpp b/modules/demux/mkv/matroska_segment.hpp
index 20ef14b..cab3702 100644
--- a/modules/demux/mkv/matroska_segment.hpp
+++ b/modules/demux/mkv/matroska_segment.hpp
@@ -27,6 +27,7 @@
 
 #include "mkv.hpp"
 #include <vector>
+#include <string>
 
 class EbmlParser;
 
@@ -49,31 +50,29 @@ typedef enum
 class SimpleTag
 {
 public:
-    SimpleTag():
-        psz_tag_name(NULL), psz_lang(NULL), b_default(true), p_value(NULL){}
-    ~SimpleTag();
-    char *psz_tag_name;
-    char *psz_lang; /* NULL value means "undf" */
-    bool b_default;
-    char * p_value;
-    std::vector<SimpleTag *> sub_tags;
+    typedef std::vector<SimpleTag> sub_tags_t;
+    std::string tag_name;
+    std::string lang;
+    std::string value;
+    sub_tags_t sub_tags;
 };
 
 class Tag
 {
 public:
+    typedef std::vector<SimpleTag> simple_tags_t;
     Tag():i_tag_type(WHOLE_SEGMENT),i_target_type(50),i_uid(0){}
-    ~Tag();
     tag_target_type i_tag_type;
     uint64_t        i_target_type;
     uint64_t        i_uid;
-    std::vector<SimpleTag*> simple_tags;
+    simple_tags_t   simple_tags;
 };
 
 class matroska_segment_c
 {
 public:
     typedef std::vector<mkv_index_t> indexes_t;
+    typedef std::vector<Tag>            tags_t;
 
     matroska_segment_c( demux_sys_t & demuxer, EbmlStream & estream );
     virtual ~matroska_segment_c();
@@ -98,7 +97,6 @@ public:
     int64_t                 i_tracks_position;
     int64_t                 i_info_position;
     int64_t                 i_chapters_position;
-    int64_t                 i_tags_position;
     int64_t                 i_attachments_position;
 
     KaxCluster              *cluster;
@@ -127,7 +125,7 @@ public:
 
     std::vector<chapter_translation_c*> translations;
     std::vector<KaxSegmentFamily*>  families;
-    std::vector<Tag *>              tags;
+    tags_t                          tags;
 
     demux_sys_t                    & sys;
     EbmlParser                     *ep;
@@ -164,7 +162,7 @@ private:
     void ParseChapterAtom( int i_level, KaxChapterAtom *ca, chapter_item_c & chapters );
     void ParseTrackEntry( KaxTrackEntry *m );
     void ParseCluster( KaxCluster *cluster, bool b_update_start_time = true, ScopeMode read_fully = SCOPE_ALL_DATA );
-    SimpleTag * ParseSimpleTags( KaxTagSimple *tag, int level = 50 );
+    bool ParseSimpleTags( SimpleTag* out, KaxTagSimple *tag, int level = 50 );
     void IndexAppendCluster( KaxCluster *cluster );
     int32_t TrackInit( mkv_track_t * p_tk );
     void ComputeTrackPriority();
-- 
2.7.2



More information about the vlc-devel mailing list