Tentative patch to enhance undo by including markers

[eumagga0x2a]

The following patch is an early but WFM attempt to implement some sort of undo tracking for markers, promised in http://avidemux.org/smif/index.php/topic,17221.0.html. It is probably too bad to be pushed and I would be deeply grateful for a preliminary review here.

Code Select Expand

diff --git a/avidemux/common/ADM_editor/include/ADM_edit.hxx b/avidemux/common/ADM_editor/include/ADM_edit.hxx
index a3f965c..8e78dbd 100644
--- a/avidemux/common/ADM_editor/include/ADM_edit.hxx
+++ b/avidemux/common/ADM_editor/include/ADM_edit.hxx
@@ -27,6 +27,8 @@
  #ifndef __ADM_composer__
  #define __ADM_composer__
  #include <string>
+ #include <utility>
+ #include <vector>

#include "ADM_assert.h"
  #include "IEditor.h"
@@ -218,7 +220,9 @@ public:
                     uint64_t    getMarkerBPts();
                     bool        setMarkerAPts(uint64_t pts);
                     bool        setMarkerBPts(uint64_t pts);
-public:
+                    std::vector <std::pair <uint64_t, uint64_t> > undoMarkersList;
+                    bool        storeMarkers(void);
+                    bool        undoMarkers(void);
/************************************ Public API ***************************/
public:
                     uint64_t    getLastKeyFramePts(void);
diff --git a/avidemux/common/ADM_editor/src/ADM_edit.cpp b/avidemux/common/ADM_editor/src/ADM_edit.cpp
index 21ce23b..9b14c38 100644
--- a/avidemux/common/ADM_editor/src/ADM_edit.cpp
+++ b/avidemux/common/ADM_editor/src/ADM_edit.cpp
@@ -515,6 +515,7 @@ uint8_t ADM_Composer::cleanup (void)
   _currentPts = 0;
   markerAPts = 0;
   markerBPts = 0;
+  undoMarkersList.clear();
   audioTrackPool.clear();
   activeAudioTracks.clear();
   return 1;
diff --git a/avidemux/common/ADM_editor/src/utils/ADM_edMarker.cpp b/avidemux/common/ADM_editor/src/utils/ADM_edMarker.cpp
index 6e0c42f..a675db7 100644
--- a/avidemux/common/ADM_editor/src/utils/ADM_edMarker.cpp
+++ b/avidemux/common/ADM_editor/src/utils/ADM_edMarker.cpp
@@ -52,4 +52,29 @@ bool        ADM_Composer::setMarkerBPts(uint64_t pts)
         markerBPts=pts;;
         return true;
}
+/**
+        \fn storeMarkers
+*/
+
+bool        ADM_Composer::storeMarkers(void)
+{
+    uint64_t a=getMarkerAPts();
+    uint64_t b=getMarkerBPts();
+    std::pair <uint64_t, uint64_t> markers=std::make_pair(a,b);
+    undoMarkersList.push_back(markers);
+    return true;
+}
+/**
+        \fn undoMarkers
+*/
+
+bool        ADM_Composer::undoMarkers(void)
+{
+    if(undoMarkersList.empty()) return false;
+    std::pair <uint64_t, uint64_t> markers=undoMarkersList.back();
+    undoMarkersList.pop_back();
+    setMarkerAPts(markers.first);
+    setMarkerBPts(markers.second);
+    return true;
+}
//EOF
diff --git a/avidemux/common/gui_main.cpp b/avidemux/common/gui_main.cpp
index ff1f664..651efdf 100644
--- a/avidemux/common/gui_main.cpp
+++ b/avidemux/common/gui_main.cpp
@@ -497,6 +497,7 @@ void HandleAction (Action action)
                   markA=markB;
                   markB=p;
               }
+              video_body->storeMarkers();
               // Special case if we are at the last frame
               bool lastFrame=false;
               uint64_t pts=video_body->getLastKeyFramePts();
@@ -541,7 +542,9 @@ void HandleAction (Action action)
             {
                 video_body->getVideoInfo(avifileinfo);
                 ReSync();
-                A_ResetMarkers();
+                if(false==video_body->undoMarkers())
+                    A_ResetMarkers();
+                A_Resync();
                 A_Rewind();

                 if(currentPts<=video_body->getVideoDuration()) GUI_GoToTime(currentPts);
@@ -600,6 +603,7 @@ void HandleAction (Action action)
                 GUI_Error_HIG(QT_TRANSLATE_NOOP("adm","Cutting"),QT_TRANSLATE_NOOP("adm","Error while cutting out."));
                 break;
             }
+            video_body->storeMarkers();
             A_ResetMarkers();
             A_Resync(); // total duration & stuff
         
@@ -842,6 +846,7 @@ int A_appendVideo (const char *name)
     uint32_t flags;
     uint64_t markerA=video_body->getMarkerAPts();
     uint64_t markerB=video_body->getMarkerBPts();
+    video_body->storeMarkers();
     video_body->getVideoPtsDts(0,&flags,&beginPts,&beginDts);
     uint64_t theEnd=video_body->getVideoDuration();
     

The behaviour this approach implements is not quite the real thing: marker A/B values get saved as pairs and added to a vector only at every paste, cut/delete and append operation, thus first performing one of these operations, altering the markers positions and finally hitting Ctrl+Z will restore markers to their values before the last paste/delete/append instead of their last position prior to performing undo.

If the patch by chance turns out to be tentatively acceptable, I'll push it to my repository and file a pull request.

TIA

[mean]

How does it relate to other operations such cutting segment / loading file etc ...

It seems like a 2nd "undo" path that is parrallel  to the old one that only stores segments

I feel like i'm missing something

[eumagga0x2a]

Segments don't know anything about markers and markers are ignorant of segments, so yes, it is a 2nd undo path which lives one floor above the segments. Undoing a cut will restore markers at the entry and the exit points of the cut, loading a file empties the vector used to store the A/B pairs.

[eumagga0x2a]

Would you prefer to try to integrate undo for markers and undo for segments into one unified call (in ADM_edit.hxx)? One possible problem with that would be that segments are much more important than markers and a failure to undo markers would either lead to undo failing in total or, if the return value of ADM_Composer::undoMarkers gets ignored, how would it be possible to call A_ResetMarkers() in ACT_Undo only if undoMarkers fails?

Under the hood it would be still the same parallel path. A redesign of the editor e.g. to include markers into segments is not a task I could accomplish while the risk/benefit ratio is very high.

edit: I did push the patch for the sake of better legibility: [Editor] Try to restore markers on undo.

[mean]

I was thinking instead of pushing the segment to the undo queue,
to push a structure {segments,markerA,markerB}

That way it is unified, whatever you do (i.e. change marker, cut, append,...) and undoing is always the same process
i.e. pop segments + markers from the undo queue

It costs a bit in terms of  memory, but capping the queue depth would fix that

[eumagga0x2a]

I'll look into that in the course of ~ a week. Thx.

[eumagga0x2a]

The following messy patch WFM, implementing a unified undo queue both for segments and markers:

Code Select Expand

commit 85cb1d4a37c73829abe993eaafe8ca42c24d7353
Author: eumagga0x2a <eumagga0x2a@users.noreply.github.com>
Date:   Fri Oct 28 22:27:20 2016 +0200

    [Editor] Implement unified undo queue for segments and markers

diff --git a/avidemux/common/ADM_editor/include/ADM_edit.hxx b/avidemux/common/ADM_editor/include/ADM_edit.hxx
index a3f965c..4b6c286 100644
--- a/avidemux/common/ADM_editor/include/ADM_edit.hxx
+++ b/avidemux/common/ADM_editor/include/ADM_edit.hxx
@@ -192,10 +192,6 @@ protected:
                                 ADM_Composer();
virtual                         ~ADM_Composer();
                     void        clean( void );
-                    bool        undo(void)
-                                {
-                                    return _segments.undo();
-                                }
                     uint8_t     resetSeg( void );
                     bool        copyToClipBoard(uint64_t startTime, uint64_t endTime);
                     bool        pasteFromClipBoard(uint64_t currentTime);
@@ -218,7 +214,22 @@ public:
                     uint64_t    getMarkerBPts();
                     bool        setMarkerAPts(uint64_t pts);
                     bool        setMarkerBPts(uint64_t pts);
+/*********************************** Undo Queue ****************************/
+struct undoQueueElem
+{
+    ListOfSegments segm;
+    uint64_t markerA;
+    uint64_t markerB;
+};
+
+typedef std::vector <undoQueueElem> ListOfUndoQueueElements;
+
+protected:
+                    ListOfUndoQueueElements undoQueue;
public:
+                    bool        addToUndoQueue(void);
+                    bool        undo(void);
+                    bool        clearUndoQueue(void);
/************************************ Public API ***************************/
public:
                     uint64_t    getLastKeyFramePts(void);
diff --git a/avidemux/common/ADM_editor/include/ADM_segment.h b/avidemux/common/ADM_editor/include/ADM_segment.h
index 67b498c..827b24b 100644
--- a/avidemux/common/ADM_editor/include/ADM_segment.h
+++ b/avidemux/common/ADM_editor/include/ADM_segment.h
@@ -17,7 +17,6 @@
#ifndef ADM_SEGMENT_H
#define ADM_SEGMENT_H
#include <vector>
-#include <list>
class ADM_audioStream;
class ADM_Audiocodec;
class decoders;
@@ -144,7 +143,6 @@ class ADM_EditorSegment
protected:
         ListOfSegments segments;
         ListOfSegments clipboard;
-        std::list <ListOfSegments> undoSegments;
         ListOfVideos   videos;
         bool           updateStartTime(void);

@@ -162,7 +160,6 @@ public:
             bool        updateRefVideo(void);
             bool        deleteAll(void);

-            bool        undo(void);
             bool        resetSegment(void);
             bool        deleteSegments(void);
             bool        addSegment(_SEGMENT *seg);
@@ -176,6 +173,9 @@ public:
             _SEGMENT    *getSegment(int i);
             int         getNbSegments(void);

+            ListOfSegments getSegments(void);
+            bool        setSegments(ListOfSegments segm);
+
             uint64_t    getTotalDuration(void);
             uint32_t    getNbFrames(void);

diff --git a/avidemux/common/ADM_editor/src/ADM_segment.cpp b/avidemux/common/ADM_editor/src/ADM_segment.cpp
index 3527701..832860b 100644
--- a/avidemux/common/ADM_editor/src/ADM_segment.cpp
+++ b/avidemux/common/ADM_editor/src/ADM_segment.cpp
@@ -170,8 +170,6 @@ bool        ADM_EditorSegment::addReferenceVideo(_VIDEOS *ref)
             ref->firstFramePts=pts;
         }

-    if(!segments.empty()) undoSegments.push_back(segments);
-
     segments.push_back(seg);
     videos.push_back(*ref);
     updateStartTime();
@@ -185,17 +183,15 @@ bool        ADM_EditorSegment::deleteSegments()
{
     ADM_info("Clearing a new segment\n");
     segments.clear();
-    undoSegments.clear();
     return true;
}
/**
-    \fn deleteSegments
-    \brief Empty the segments list
+    \fn addSegment
+    \brief Add a segment to the segments list
*/
bool        ADM_EditorSegment::addSegment(_SEGMENT *seg)
{
     ADM_info("Adding a new segment\n");
-    undoSegments.push_back(segments);
     segments.push_back(*seg);
     updateStartTime();
     return true;
@@ -253,21 +249,6 @@ bool ADM_EditorSegment::deleteAll (void)
     clipboard.clear();
     videos.clear();
     segments.clear();
-    undoSegments.clear();
-    return true;
-}
-
-
-/**
-    \fn undo
-    \brief
-*/
-bool        ADM_EditorSegment::undo(void)
-{
-    if(undoSegments.empty()) return false;
-    segments=undoSegments.back();
-    undoSegments.pop_back();
-    updateStartTime();
     return true;
}
/**
@@ -279,7 +260,6 @@ bool        ADM_EditorSegment::resetSegment(void)
     //
     aviInfo info;
     segments.clear();
-    undoSegments.clear();
     int n=videos.size();
     for(int i=0;i<n;i++)
     {
@@ -323,6 +303,29 @@ _VIDEOS     *ADM_EditorSegment::getRefVideo(int i)
     return &(videos[i]);
}
/**
+    \fn getSegments
+    \brief getter for the list of segments
+*/
+ListOfSegments ADM_EditorSegment::getSegments(void)
+{
+    if(segments.empty()) ADM_assert(0);
+    ListOfSegments segm=segments;
+    return segm;
+}
+/**
+    \fn setSegments
+    \brief setter for the list of segments
+*/
+bool ADM_EditorSegment::setSegments(ListOfSegments segm)
+{
+    if(segm.size())
+    {
+        segments=segm;
+        return true;
+    }
+    return false;
+}
+/**
     \fn getNbRefVideo
     \brief getNbRefVideo
*/
@@ -614,7 +617,6 @@ bool        ADM_EditorSegment::removeChunk(uint64_t from, uint64_t to)
         return false;

     }
-    undoSegments.push_back(tmp);
     dump();
     return true;
}
@@ -656,7 +658,6 @@ bool ADM_EditorSegment::truncateVideo(uint64_t from)
         updateStartTime();
         return false;
     }
-    undoSegments.push_back(tmp);
     dump();
     return true;
}
@@ -934,7 +935,6 @@ bool        ADM_EditorSegment::pasteFromClipBoard(uint64_t currentTime)
     }
     segments=newSegs;
     updateStartTime();
-    undoSegments.push_back(tmp);
     dump();
     return true;
}
@@ -955,7 +955,6 @@ bool ADM_EditorSegment::appendFromClipBoard(void)
     ListOfSegments tmp=segments;
     for(int i=0;i<clipboard.size();i++) segments.push_back(clipboard[i]);
     updateStartTime();
-    undoSegments.push_back(tmp);
     dump();
     return true;
}
diff --git a/avidemux/common/ADM_editor/src/CMakeLists.txt b/avidemux/common/ADM_editor/src/CMakeLists.txt
index d45ed94..9bbd65c 100644
--- a/avidemux/common/ADM_editor/src/CMakeLists.txt
+++ b/avidemux/common/ADM_editor/src/CMakeLists.txt
@@ -15,6 +15,7 @@ utils/ADM_editIface.cpp
utils/ADM_edScriptGenerator.cpp
utils/ADM_edFrameType.cpp
utils/ADM_edCache.cpp
+utils/ADM_edUndoQueue.cpp
audio/ADM_edEditableAudioTrack.cpp
audio/ADM_edPoolOfAudioTracks.cpp
audio/ADM_edActiveAudioTracks.cpp
@@ -26,4 +27,4 @@ audio/ADM_edAudioTrack.cpp
)

ADD_LIBRARY(ADM_editor6 STATIC ${ADM_editor_SRCS})
-include_directories(../ADM_script2/include)
+include_directories(../ADM_script2/include)
diff --git a/avidemux/common/ADM_editor/src/utils/ADM_edUndoQueue.cpp b/avidemux/common/ADM_editor/src/utils/ADM_edUndoQueue.cpp
new file mode 100644
index 0000000..d7405c3
--- /dev/null
+++ b/avidemux/common/ADM_editor/src/utils/ADM_edUndoQueue.cpp
@@ -0,0 +1,76 @@
+/** *************************************************************************
+     \file       ADM_edUndoQueue.cpp
+     \brief      Handle Undo
+
+ ***************************************************************************/
+
+/***************************************************************************
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) any later version.                                   *
+ *                                                                         *
+ ***************************************************************************/
+#include "ADM_cpp.h"
+#include "ADM_default.h"
+#include "ADM_segment.h"
+#include "ADM_edit.hxx"
+
+static const uint8_t maxUndoSteps=50;
+
+/**
+    \fn addToUndoQueue
+    \brief stores the segment layout and markers in the undo queue
+*/
+
+bool ADM_Composer::addToUndoQueue(void)
+{
+    undoQueueElem rec;
+    uint64_t a=getMarkerAPts();
+    uint64_t b=getMarkerBPts();
+    rec.segm=_segments.getSegments();
+    rec.markerA=a;
+    rec.markerB=b;
+    uint32_t m=maxUndoSteps;
+    if(m<10) m=10; // less than 10 undo steps is a bad idea, ignore the limit then
+    uint32_t nb=undoQueue.size();
+    if(nb>m)
+    {
+        // erase the oldest records if the limit is exceeded and create space for a new one
+        undoQueue.erase(undoQueue.begin(),undoQueue.begin()+nb-m+1);
+    }
+    undoQueue.push_back(rec);
+    return true;
+}
+
+/**
+    \fn undo
+    \brief restores the last recorded state of segments and markers
+*/
+
+bool ADM_Composer::undo(void)
+{
+    if(undoQueue.empty())
+    {
+        ADM_info("The undo queue is empty, nothing to do\n");
+        return false;
+    }
+    undoQueueElem rec=undoQueue.back();
+    _segments.setSegments(rec.segm);
+    setMarkerAPts(rec.markerA);
+    setMarkerBPts(rec.markerB);
+    undoQueue.pop_back();
+    return true;
+}
+
+/**
+    \fn clearUndoQueue
+*/
+
+bool ADM_Composer::clearUndoQueue(void)
+{
+    undoQueue.clear();
+    return true;
+}
+//EOF
diff --git a/avidemux/common/gui_main.cpp b/avidemux/common/gui_main.cpp
index ff1f664..6984809 100644
--- a/avidemux/common/gui_main.cpp
+++ b/avidemux/common/gui_main.cpp
@@ -465,6 +465,7 @@ void HandleAction (Action action)
           markA=markB;
           markB=y;
         }
+        video_body->addToUndoQueue();
         video_body->setMarkerAPts(markA);
         video_body->setMarkerBPts(markB);
         UI_setMarkers (markA, markB);
@@ -497,6 +498,7 @@ void HandleAction (Action action)
                   markA=markB;
                   markB=p;
               }
+              video_body->addToUndoQueue();
               // Special case if we are at the last frame
               bool lastFrame=false;
               uint64_t pts=video_body->getLastKeyFramePts();
@@ -540,8 +542,7 @@ void HandleAction (Action action)
             if(video_body->undo())
             {
                 video_body->getVideoInfo(avifileinfo);
-                ReSync();
-                A_ResetMarkers();
+                A_Resync();
                 A_Rewind();

                 if(currentPts<=video_body->getVideoDuration()) GUI_GoToTime(currentPts);
@@ -580,7 +581,7 @@ void HandleAction (Action action)
             {
                 video_body->copyToClipBoard(a,b);
             }
-
+            video_body->addToUndoQueue();
             // Special case of B being at or beyond the last frame
             bool lastFrame=false;
             bool result=false;
@@ -862,6 +863,7 @@ int A_appendVideo (const char *name)

     if (playing)
         return 0;
+    video_body->addToUndoQueue();
     if (!video_body->addFile (name))
     {
       GUI_Error_HIG (QT_TRANSLATE_NOOP("adm","Something failed when appending"), NULL);
@@ -1431,6 +1433,7 @@ uint8_t GUI_close(void)
       //delete wavinfo;
       admPreview::destroy();
       avifileinfo = NULL;
+      video_body->clearUndoQueue();
       video_body->cleanup ();

//      filterCleanUp ();

The patch is almost for sure not fit for production, but could you please have a look? The idea was to move undo out of segment management one level higher into the composer and call addToUndoQueue(), undo() and clearUndoQueue() from gui_main.cpp when necessary. The maximum length of undo queue could become a preference setting later.

I don't know if putting declaration stuff for undo queue straight into ADM_edit.hxx instead of an additional header file is acceptable or a terrible mauvais ton, I'd be deeply grateful for some guidance how to do this right.

Testing Avidemux with this patch, I felt that the ability to undo changes to marker placement made me thirst for Redo feature. But I'd like to get this step right first, then extend the functionality further.

edit: got git finally right, my apologies for the initial confusion.

[mean]

Thank,   I ll process it  as soon  as  i  m  back home

[eumagga0x2a]

I'm sorry, I didn't check the topic prior to opening the last pull request. Under no circumstances I wanted to press you. TIA and sorry again.

[mean]

No  problem,  i m  away for  a  few  days

[eumagga0x2a]

Thank you very much for merging the undo/redo patch. If you have points of criticism regarding the implementation, I'd be eager to hear about them and to learn for future contributions.