Delete the last frame of a video

[eumagga0x2a]

I'm very sorry, but I'd really love to see 2.6.15 with FFmpeg 3.0.3 pushed out of the door ASAP just once these basic fixes for the basic user facing functionality (editor and navigation) are done, with major groundwork, refactoring of parts of GUI and polish delayed for 2.6.16.

[eumagga0x2a]

There a general design flaw when the video does not start at zero
It might be worth it to fix for good

A hint to the direction of the design flaw would be highly appreciated

[mean]

Basically it is assumed +- that the video starts at zero
So the code contains portions that assume goTo(0) is legit, and tries to compensate later. As a result you end up with discrepencies between the former and the later.

Similarily the duration is not always correctly computed due to the above.

I have a test case somewhere where it is very wrong, dont know if i still have it

[eumagga0x2a]

Thank you very much for the valuable insight. Yes, the duration calculation at least in avidemux_core/ADM_coreDemuxer/include/ADM_coreDemuxerMpegTemplate.cpp.h in MY_CLASS::getVideoDuration indeed returns strictly speaking not the duration but maxPTS of the video. The duration would be

maxPTS-firstFramePts

then. But other demuxers like the Matroska one seem to rely just on the container header, right?

While I'll do my best to explore in my now greatly reduced spare time how far I could get in this task, I would argue that this should not delay let alone block the release of 2.6.15 with already available bits improving the core functionality and user experience. The patches shouldn't rely on buggy behaviour, sure, but most of them will be needed even with the duration and initial marker position calculation fixed or won't do any harm at least.

[mean]

I'm not sure if i missed a patch or not ?

[eumagga0x2a]

I can't say for sure. It was not immediately clear whether http://avidemux.org/smif/index.php/topic,16935.msg77308.html#msg77308 was missed or rejected (no problem with that, but it would be immensly helpful for a total newbie amater like me to understand my mistakes and try harder next time). The rationale for that patch was: If we can't rely on the initial marker B to be the exact PTS of the last frame for now, don't throw an error at the user and do the least wrong thing silently. But maybe things like bailing out on pts==ADM_NO_PTS are non-starters anyway (and there are trailing whitespaces at that line copypasted from gui_navigate.cpp:152 as well...).

WRT to deleting the last frame, I think a tailored function (the first patch editor-truncate-fn.patch from http://avidemux.org/smif/index.php/topic,16935.msg77368.html#msg77368) would be a more solid approach if the marker B is designed not to be beyond the last frame later. The second patch there I'd replace with a slightly nicer version

Code Select Expand

diff --git a/avidemux/common/gui_main.cpp b/avidemux/common/gui_main.cpp
index 1a73404..f8c9d2a 100644
--- a/avidemux/common/gui_main.cpp
+++ b/avidemux/common/gui_main.cpp
@@ -560,7 +560,21 @@ void HandleAction (Action action)
                 video_body->copyToClipBoard(a,b);
             }

-            if(false==video_body->remove(a,b))
+            // Special case of B being at or beyond the last frame
+            bool lastFrame=false;
+            uint64_t pts=video_body->getLastKeyFramePts();
+            if(pts!=ADM_NO_PTS && b>=pts) // don't waste time if B is before the last keyframe
+            {
+                admPreview::deferDisplay(1);
+                GUI_GoToTime(pts);
+                while(admPreview::nextPicture())
+                {
+                }
+                admPreview::deferDisplay(0);
+                uint64_t lastFramePts=video_body->getCurrentFramePts();
+                if(b>=lastFramePts) lastFrame=true; // B is at or beyond the last frame
+            }
+            if((lastFrame==true && false==video_body->truncate(a)) || (lastFrame==false && false==video_body->remove(a,b)))
             {
                 GUI_Error_HIG(QT_TRANSLATE_NOOP("adm","Cutting"),QT_TRANSLATE_NOOP("adm","Error while cutting out."));
                 break;

(this patch is attached).

Unrelated bits which would be nice to have once the current work on FDK is completed are [UI] Preserve markers A/B and the current position when appending video and Re: Loading (not appending) a new video resets the encoder settings to "Copy" (the latter patch makes the issue with redrawing the Avidemux window before internal resizing is completed more visible, but I think that it is worth the trouble).

Would you like the position slider to represent the real PTS or the offset (currentPts-firstFramePts), making all videos start at zero, in the future?

Thank you for your patience.

[mean]

I think i've taken them all, please say so if i missed something again

[eumagga0x2a]

Mille mercis ââ,¬â€œ especially for getting rid of that monster "if" one-liner in the patch for gui_main.cpp choosing between remove(a,b) and truncate(a) and for avoiding code duplication thanks to the new GUI_infiniteForward function. Everything works as expected.

One patch didn't make it into the repository yet: Re: Loading (not appending) a new video resets the encoder settings to "Copy". Maybe it was missed. While it still applies, it applies at an offset due to recent commits. I've attached a version of this minimal patch (preserve-codec-settings-on-loading-video.patch) with adjusted line numbers here.

[mean]

That one is debatable
Restting to "default conf" is fine

Some people find it logical to set codec first, load afterward
Others do not, and prefer to start from a clean state

[eumagga0x2a]

I'll try to explain. Any default configuration different from copy/copy/avi requires a conscious decision by the user (who is likely unaware that the audio codec settings won't be saved by "Save current settings as default", by the way). Punishing the user for not having taken this action at the right moment by discarding user generated codec and container configuration without any notice could cause surprise, confusion and anger. IMHO it would be better to preserve the settings across videos and provide an easy way to revert to default settings instead, e.g.:

Code Select Expand

diff --git a/avidemux/common/ADM_commonUI/myOwnMenu.h b/avidemux/common/ADM_commonUI/myOwnMenu.h
index fb29e07..2b16b48 100644
--- a/avidemux/common/ADM_commonUI/myOwnMenu.h
+++ b/avidemux/common/ADM_commonUI/myOwnMenu.h
@@ -63,6 +63,7 @@ static const MenuEntry _myMenuEdit[] = {
             {MENU_ACTION,QT_TRANSLATE_NOOP("adm","Pr&eferences"),       NULL,ACT_PREFERENCES,NULL,NULL},
             {MENU_SEPARATOR,"-",NULL,ACT_DUMMY             ,NULL,NULL},
             {MENU_ACTION,QT_TRANSLATE_NOOP("adm","Save current settings as default"), NULL,ACT_SaveAsDefault,NULL,NULL},
+            {MENU_ACTION,QT_TRANSLATE_NOOP("adm","Load saved settings"),              NULL,ACT_LoadDefault,  NULL,NULL}
         };

std::vector<MenuEntry> myMenuEdit(_myMenuEdit, _myMenuEdit + sizeof(_myMenuEdit) / sizeof(_myMenuEdit[0]));
diff --git a/avidemux/common/gui_action.names b/avidemux/common/gui_action.names
index deab7ef..5367d9b 100644
--- a/avidemux/common/gui_action.names
+++ b/avidemux/common/gui_action.names
@@ -116,4 +116,4 @@ ACT(RUN_SCRIPT)
ACT(SAVE_PY_SCRIPT)
//
ACT(SaveAsDefault)
-       
+ACT(LoadDefault)
diff --git a/avidemux/common/gui_main.cpp b/avidemux/common/gui_main.cpp
index 645f3fd..9e14a45 100644
--- a/avidemux/common/gui_main.cpp
+++ b/avidemux/common/gui_main.cpp
@@ -160,6 +160,11 @@ void HandleAction (Action action)
         A_saveDefaultSettings();
         return;
     }
+    if(action==ACT_LoadDefault)
+    {
+        A_loadDefaultSettings();
+        return;
+    }
     if (action >= ACT_SCRIPT_ENGINE_FIRST && action < ACT_SCRIPT_ENGINE_LAST)
     {
         int engineIndex = (action - ACT_SCRIPT_ENGINE_FIRST) / 3;


The issue with this tentative patch is that A_loadDefaultSettings doesn't work while playing, and an improvised attempt to automate

Code Select Expand

    if(action==ACT_LoadDefault)
    {
        bool restore=false;
        if(playing)
        {
            GUI_PlayAvi();
            restore=true;
        }
        A_loadDefaultSettings();
        if(restore) GUI_PlayAvi();
        return;
    }

just stopps playback and that's all.

The problem with audio codec settings not saved by A_saveDefaultSettings should be probably evaluated separately, especially considering the handling of multiple audio tracks with individual settings per track.

[mean]

Sorry for not being clearer
What i had in mind was a preference setting, where the user can choose the behaviour upon loading a video
i.e.
Keep settings
Reset to default (i.e. clear then load default)

I guess that would please everyone ?

[eumagga0x2a]

I'll try to explain better. On one hand, a need for Avidemux to revert to a saved default upon loading a video implies that the codec and output container config was modified for the previous video in the first place, which itself can be regarded rather an exception than a rule. On the other hand, the cost in additional clicks, time and attention to restore an unintentionally discarded custom codec config is incommensurate with two clicks to reach an option in the Edit menu (maybe add a button to the currently almost empty toolbar too?). I'd argue that we should avoid including a preference setting which might likely result in an unwanted dataloss (counting user defined config to data).

An easy discoverable option to reset codec settings to default would be handy anyway e.g. if a user has set markers A and B before realizing that a modified codec config is not good (I don't count loading a project script defaultSettings.py as an easy discoverable way to accomplish this task). Closing the video would discard the markers.

There are two real issues now regardless of a solution with or without a preference setting:

• Audio codec settings don't get saved and thus can't be restored, leading to conflicts with some output containers.
• User defined codec settings not yet saved as default do get discarded without notice on exit.

Both are not that kind of low hanging fruit I could easily pick, unfortunately.

The current state of affairs described in http://avidemux.org/smif/index.php/topic,17014.msg76321.html#msg76321 is worse than any of discussed solutions though. The situation with filters being discarded no matter what is probably not optimal either.

[eumagga0x2a]

Maybe automatic saving of codec and filter configuration to config3 file with an option to save the current config as a preset and an easy way to load presets could be implemented some day in the future.

[eumagga0x2a]

By the way, why not adding keyboard shortcuts for "Save current settings as default" (e.g. Ctrl+Alt+D) and for tentative "Load saved settings" option (e.g. Ctrl+R)? This would make these options even more accessible.

Code Select Expand

diff --git a/avidemux/common/ADM_commonUI/myOwnMenu.h b/avidemux/common/ADM_commonUI/myOwnMenu.h
index fb29e07..42c3467 100644
--- a/avidemux/common/ADM_commonUI/myOwnMenu.h
+++ b/avidemux/common/ADM_commonUI/myOwnMenu.h
@@ -62,7 +62,8 @@ static const MenuEntry _myMenuEdit[] = {
             {MENU_SEPARATOR,"-",NULL,ACT_DUMMY             ,NULL,       NULL},
             {MENU_ACTION,QT_TRANSLATE_NOOP("adm","Pr&eferences"),       NULL,ACT_PREFERENCES,NULL,NULL},
             {MENU_SEPARATOR,"-",NULL,ACT_DUMMY             ,NULL,NULL},
-            {MENU_ACTION,QT_TRANSLATE_NOOP("adm","Save current settings as default"), NULL,ACT_SaveAsDefault,NULL,NULL},
+            {MENU_ACTION,QT_TRANSLATE_NOOP("adm","Save current settings as default"), NULL,ACT_SaveAsDefault,NULL,"Ctrl+Alt+D"},
+            {MENU_ACTION,QT_TRANSLATE_NOOP("adm","Load saved settings"),              NULL,ACT_LoadDefault,  NULL,"Ctrl+R"}
         };

std::vector<MenuEntry> myMenuEdit(_myMenuEdit, _myMenuEdit + sizeof(_myMenuEdit) / sizeof(_myMenuEdit[0]));
diff --git a/avidemux/common/gui_action.names b/avidemux/common/gui_action.names
index deab7ef..5367d9b 100644
--- a/avidemux/common/gui_action.names
+++ b/avidemux/common/gui_action.names
@@ -116,4 +116,4 @@ ACT(RUN_SCRIPT)
ACT(SAVE_PY_SCRIPT)
//
ACT(SaveAsDefault)
-       
+ACT(LoadDefault)
diff --git a/avidemux/common/gui_main.cpp b/avidemux/common/gui_main.cpp
index 645f3fd..9e14a45 100644
--- a/avidemux/common/gui_main.cpp
+++ b/avidemux/common/gui_main.cpp
@@ -160,6 +160,11 @@ void HandleAction (Action action)
         A_saveDefaultSettings();
         return;
     }
+    if(action==ACT_LoadDefault)
+    {
+        A_loadDefaultSettings();
+        return;
+    }
     if (action >= ACT_SCRIPT_ENGINE_FIRST && action < ACT_SCRIPT_ENGINE_LAST)
     {
         int engineIndex = (action - ACT_SCRIPT_ENGINE_FIRST) / 3;


[eumagga0x2a]

Thank you for taking the patch adding the reload saved default settings menu option, I'll look into adding a new preference setting over the course of the week to learn how this is done.

Reading ADM_edScriptGenerator WRT to GENERATE_SETTINGS vs. GENERATE_ALL, it looks like it was a (sort of forced) cost-benefit decision to exclude audio encoder settings from A_saveDefaultSettings.