Is it possible to delete the last frame of a video? I noticed that after setting A and B and cutting, frame B doesn't get deleted for some reason, only the frames before it. Copy+pasting a section of the video and pasting at the end of the video just puts it before the last frame, so that doesn't work either. I'm using Avidemux 2.6.12 with an AVI file.
Old avi? Try with Avidemux 2.5.6 (frame based editor), 2.6 branch is time based to cope with newer codecs. Otherwise try re-encoding if you are using [Copy] for video
Quote from: 2739480298184121 on July 15, 2016, 05:42:32 PM
Is it possible to delete the last frame of a video? I noticed that after setting A and B and cutting, frame B doesn't get deleted for some reason, only the frames before it. Copy+pasting a section of the video and pasting at the end of the video just puts it before the last frame, so that doesn't work either. I'm using Avidemux 2.6.12 with an AVI file.
Just set A, leave B unset. In this way you will able to cut video from A to video end.
I dig this thread out because both the issue and the workaround is probably a bug worth fixing. Many videos don't start at zero, the first frame has a small offset. The initial value for B is the total duration of the video which includes this initial offset, but navigating to the last frame (hitting the END key) shows the time without this offset. Setting the B marker now places it before the last frame.
Quote from: eumagga0x2a on September 24, 2016, 05:52:18 PMThe initial value for B is the total duration of the video which includes this initial offset, but navigating to the last frame (hitting the END key) shows the time without this offset. Setting the B marker now places it before the last frame.
Other way round: loading a video and hitting PgDown results in "Error seeking to ........ ms" error dialog where the # of milliseconds is the time corresponding to the initial value of the B marker. This time, returned by ADM_Composer::getVideoDuration, reaches beyond the last frame of the video. Navigating to the last frame goes to the time which should be used as the initial value for B.
If ADM_EditorSegment::removeChunk would be extended to deal with a special case of the B marker being at the last frame of the video, everything would be fine.
The following first rough approximation allows ADM_EditorSegment::removeChunk to delete the last frame(s) of a video:
diff --git a/avidemux/common/ADM_editor/src/ADM_segment.cpp b/avidemux/common/ADM_editor/src/ADM_segment.cpp
index 2209c88..2c128e1 100644
--- a/avidemux/common/ADM_editor/src/ADM_segment.cpp
+++ b/avidemux/common/ADM_editor/src/ADM_segment.cpp
@@ -581,7 +581,16 @@ bool ADM_EditorSegment::removeChunk(uint64_t from, uint64_t to)
ADM_info("Start, seg %" PRIu32" Offset :%" PRIu64" ms\n",startSeg,startOffset);
ADM_info("End , seg %" PRIu32" Offset :%" PRIu64" ms\n",endSeg,endOffset);
ListOfSegments tmp=segments;
-
+
+ // Special case at the end of the video
+ _VIDEOS *v=getRefVideo(0);
+ uint64_t pts=v->firstFramePts;
+ if(pts<50000) pts=50000; // kill the last segment < 50ms for sure
+ bool eov=false;
+ if(to>=getTotalDuration()-pts)
+ {
+ eov=true;
+ }
if(startSeg==endSeg)
{
@@ -599,14 +608,21 @@ bool ADM_EditorSegment::removeChunk(uint64_t from, uint64_t to)
// 3- Shorten last segment
_SEGMENT *last=getSegment(endSeg);
last->_refStartTimeUs+=endOffset;
- last->_durationUs-=endOffset;
+ if(!eov)
+ {
+ last->_durationUs-=endOffset;
+ }
+ else
+ {
+ last->_durationUs=0;
+ }
// 2- Kill the segment in between
for(int i=startSeg+1;i<endSeg;i++)
{
segments.erase(segments.begin()+startSeg+1);
}
- updateStartTime();
removeEmptySegments();
+ updateStartTime();
if(isEmpty())
{
GUI_Error_HIG(QT_TRANSLATE_NOOP("adm","Error"),QT_TRANSLATE_NOOP("adm","You cannot remove *all* the video\n"));
The currently necessary hack with minimum value of 50ms for pts is terrible, it should really find out and use the PTS of the last frame instead.
I'll need a bit of time to check that one
Unfortunately, the first part of the patch ââ,¬â€œ determining if we are going to delete everything past the marker A ââ,¬â€œ can't go into production. I already ran into enough videos with total duration much higher than the PTS of the last frame (160ms max. difference so far) with a very small non-zero start PTS, so assuming that 50ms might be enough is wrong.
It looks for me like there is no easy way to teach ADM_EditorSegment::removeChunk to decide on its own whether it should remove a chunk or rather truncate a video. The logic would have to move a level or two higher then and probably require a custom function.
With regard to the nasty error when trying to go to marker B value automatically set after loading a video, I would suggest to work around the issue via something like
diff --git a/avidemux/common/gui_navigate.cpp b/avidemux/common/gui_navigate.cpp
index 23df368..29e6c0c 100644
--- a/avidemux/common/gui_navigate.cpp
+++ b/avidemux/common/gui_navigate.cpp
@@ -90,8 +93,26 @@ static int ignore_change=0;
uint64_t pts;
if(action==ACT_GotoMarkA) pts=video_body->getMarkerAPts();
else pts=video_body->getMarkerBPts();
- GUI_GoToTime(pts);
-
+ if(false==video_body->goToTimeVideo(pts))
+ {
+ if(action==ACT_GotoMarkA)
+ {
+ ADM_warning("Go to Marker A: Seek to time %s ms failed\n",ADM_us2plain(pts));
+ }else // PTS returned by getMarkerBPts() may be beyond the last frame.
+ // Go to the last frame then.
+ {
+ pts=video_body->getLastKeyFramePts();
+ if(pts==ADM_NO_PTS) break;
+ admPreview::deferDisplay(1);
+ GUI_GoToTime(pts);
+ while(admPreview::nextPicture())
+ {
+ }
+ admPreview::deferDisplay(0);
+ }
+ }
+ admPreview::samePicture();
+ GUI_setCurrentFrameAndTime();
}
break;
in gui_navigate.cpp. The present GUI_GoToTime function can't be used in the test here because it always returns true. The rest is copypasted from the ACT_End action.
The first of the two patches adds a function ADM_Composer::truncate and the second one extends the logic of ACT_delete to handle the special case of the B marker being at or beyond the last frame and to choose between remove(from,to) and truncate(from) accordingly.
The only but important issue I have found so far is that getLastKeyFramePts still somehow sees and reports last keyframe beyond the end of the truncated video, messing up functions like ACT_End after truncating. It means, the patches are not yet ripe for production. Apart from that, they work for me.
diff --git a/avidemux/common/ADM_editor/include/ADM_edit.hxx b/avidemux/common/ADM_editor/include/ADM_edit.hxx
index 3a29198..5c18f16 100644
--- a/avidemux/common/ADM_editor/include/ADM_edit.hxx
+++ b/avidemux/common/ADM_editor/include/ADM_edit.hxx
@@ -275,6 +275,7 @@ public:
/******************************* /Post Processing ************************************/
/******************************* Editing ************************************/
bool remove(uint64_t start,uint64_t end);
+ bool truncate(uint64_t start);
bool addSegment(uint32_t ref, uint64_t startRef, uint64_t duration);
bool clearSegment(void);
uint32_t getNbSegment(void)
diff --git a/avidemux/common/ADM_editor/include/ADM_segment.h b/avidemux/common/ADM_editor/include/ADM_segment.h
index c6f16f5..ce3d37a 100644
--- a/avidemux/common/ADM_editor/include/ADM_segment.h
+++ b/avidemux/common/ADM_editor/include/ADM_segment.h
@@ -189,6 +189,7 @@ public:
bool isKeyFrameByTime(uint32_t refVideo,uint64_t seekTime);
bool removeChunk(uint64_t from, uint64_t to);
+ bool truncateVideo(uint64_t from);
bool dtsFromPts(uint32_t refVideo,uint64_t pts,uint64_t *dts);
bool LinearToRefTime(int segNo,uint64_t linear,uint64_t *refTime);
diff --git a/avidemux/common/ADM_editor/src/ADM_edit.cpp b/avidemux/common/ADM_editor/src/ADM_edit.cpp
index dab497f..9268698 100644
--- a/avidemux/common/ADM_editor/src/ADM_edit.cpp
+++ b/avidemux/common/ADM_editor/src/ADM_edit.cpp
@@ -682,6 +682,14 @@ bool ADM_Composer::remove(uint64_t start,uint64_t end)
return _segments.removeChunk(start,end);
}
/**
+ \fn truncate
+ \brief Truncate video
+*/
+bool ADM_Composer::truncate(uint64_t start)
+{
+ return _segments.truncateVideo(start);
+}
+/**
\fn dumpEditing
\brief Dump segment, video & al
*/
diff --git a/avidemux/common/ADM_editor/src/ADM_segment.cpp b/avidemux/common/ADM_editor/src/ADM_segment.cpp
index 2209c88..b100f6f 100644
--- a/avidemux/common/ADM_editor/src/ADM_segment.cpp
+++ b/avidemux/common/ADM_editor/src/ADM_segment.cpp
@@ -619,8 +619,48 @@ bool ADM_EditorSegment::removeChunk(uint64_t from, uint64_t to)
dump();
return true;
}
+/**
+ \fn truncateVideo
+ \brief Remove a part of the video from the given time to the end
+*/
+bool ADM_EditorSegment::truncateVideo(uint64_t from)
+{
+ uint32_t startSeg;
+ uint64_t startOffset;
+ ADM_info("Truncating from %" PRIu64" ms\n",from/1000);
+ dump();
+ if(false==convertLinearTimeToSeg(from,&startSeg,&startOffset))
+ {
+ ADM_warning("Cannot get starting point for linear time %" PRIu64" ms\n",from/1000);
+ return false;
+ }
+ ADM_info("Start in segment %" PRIu32" at offset :%" PRIu64" ms\n",startSeg,startOffset);
+ ListOfSegments tmp=segments;
+
+ _SEGMENT *first=getSegment(startSeg);
+ // shorten the start segment
+ first->_durationUs=startOffset;
+ // remove following segments
+ int n=segments.size();
+ for(int i=startSeg+1;i<n;i++)
+ {
+ segments.erase(segments.begin()+startSeg+1);
+ }
+ updateStartTime();
+ removeEmptySegments();
+ if(isEmpty())
+ {
+ GUI_Error_HIG(QT_TRANSLATE_NOOP("adm","Error"),QT_TRANSLATE_NOOP("adm","You cannot remove *all* the video\n"));
+ segments=tmp;
+ updateStartTime();
+ return false;
+ }
+ undoSegments.push_back(tmp);
+ dump();
+ return true;
+}
/**
\fn dump
\brief Dump the segment content
diff --git a/avidemux/common/gui_main.cpp b/avidemux/common/gui_main.cpp
index 1a73404..d72045f 100644
--- a/avidemux/common/gui_main.cpp
+++ b/avidemux/common/gui_main.cpp
@@ -560,7 +560,24 @@ 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 atTheEnd=false;
+ uint64_t pts=video_body->getLastKeyFramePts();
+ if(pts!=ADM_NO_PTS)
+ {
+ if(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) atTheEnd=true; // B is at or beyond the last frame
+ }
+ }
+ if((atTheEnd==true && false==video_body->truncate(a)) || (atTheEnd==false && false==video_body->remove(a,b)))
{
GUI_Error_HIG(QT_TRANSLATE_NOOP("adm","Cutting"),QT_TRANSLATE_NOOP("adm","Error while cutting out."));
break;
I'm not sure i get that line
first->_durationUs=startOffset;
Thank you for looking at the patch. This part is copypasted from ADM_EditorSegment::removeChunk straight above and says that the segment where the marker A happens to be should not extend beyond this point (represented by startOffset), the duration of the segment must be equal the marker A offset in relation to the beginning of this segment.
I still haven't understood why ADM_Composer::getLastKeyFramePts gets messed up once ADM_EditorSegment::truncateVideo has been executed (it returns the same PTS as before truncating).
The thing is convertLinear will give the the segment + offset inside the segment of the new "end"
That segment duration will be equal to the offset only if the segment is taken from its beginning
Example :
Linear 0..10 => actually it's segment 0 from 10 to 20
Truncate (8) => segment 0, end point = 18
Duration=18
Where we want to adjust the duration i.e. correct duration=8
(Unless i'm not full awake)
Quote from: mean on October 03, 2016, 05:48:31 AM
The thing is convertLinear will give the the segment + offset inside the segment of the new "end"
That segment duration will be equal to the offset only if the segment is taken from its beginning
Example :
Linear 0..10 => actually it's segment 0 from 10 to 20
Wouldn't it imply _startTimeUs, which is declared as uint64_t, going negative? In my very limited understanding the example above may apply to time in reference (_refStartTimeUs) which can be e.g. 10 at linear 0 while time in the first segment must be 0 at linear 0. I do see question marks in
if(!frameTime && segments.size()) // pick the first one
{
ADM_info("Frame time=0, taking first segment \n");
*seg=0;
*segTime=0; // ??
return true;
}in the body of ADM_EditorSegment::convertLinearTimeToSeg at ADM_segment.cpp:450 though...
In any case the current removeChunk does exactly the same for the segment containing the A marker apparently without any issues.
QuoteTruncate (8) => segment 0, end point = 18
Duration=18
Where we want to adjust the duration i.e. correct duration=8
In my again extremely limited understanding there is no need to safeguard for time in segment > linear.
The real problem seems to exist in ADM_Composer::getLastKeyFramePts, because it currently can't handle the case of end of the video != the end of the reference video (apart from misleading ADM_info which doesn't report what the function actually returns). WRT to the info printed to the console I use currently a private patch
diff --git a/avidemux/common/ADM_editor/src/ADM_edSearch.cpp b/avidemux/common/ADM_editor/src/ADM_edSearch.cpp
index df4385f..0edbe05 100644
--- a/avidemux/common/ADM_editor/src/ADM_edSearch.cpp
+++ b/avidemux/common/ADM_editor/src/ADM_edSearch.cpp
@@ -388,12 +388,14 @@ uint64_t ADM_Composer::getLastKeyFramePts(void)
if(!(flags & AVI_KEY_FRAME)) continue;
v->_aviheader->getPtsDts(frame,&pts,&dts);
if(pts==ADM_NO_PTS) continue;
- ADM_info("found last keyframe at %d, time=%s\n",frame,ADM_us2plain(pts));
+ ADM_info("found last keyframe at %d, time in reference=%s\n",frame,ADM_us2plain(pts));
break;
}
if(pts!=ADM_NO_PTS)
{
- return pts+s->_startTimeUs-s->_refStartTimeUs;
+ pts+=s->_startTimeUs-s->_refStartTimeUs;
+ ADM_info("found last keyframe at %s\n",ADM_us2plain(pts));
+ return pts;
}
}
ADM_info("Cannot find lastKeyFrame with a valid PTS\n");Dropping the idea of a tailored function for truncating video and simply using
diff --git a/avidemux/common/gui_main.cpp b/avidemux/common/gui_main.cpp
index 1a73404..ef29170 100644
--- a/avidemux/common/gui_main.cpp
+++ b/avidemux/common/gui_main.cpp
@@ -560,6 +560,20 @@ void HandleAction (Action action)
video_body->copyToClipBoard(a,b);
}
+ // Special case of B being at the last frame
+ 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();
+ // put B beyond the last frame to ensure that it will be removed too
+ if(b>=lastFramePts) b=video_body->getVideoDuration();
+ }
if(false==video_body->remove(a,b))
{
GUI_Error_HIG(QT_TRANSLATE_NOOP("adm","Cutting"),QT_TRANSLATE_NOOP("adm","Error while cutting out."));(edited: nonsensical nested ifs in the patch removed, sorry) to handle the case when B is at the last frame results in the same issues originating from the current implementation of ADM_Composer::getLastKeyFramePts.
After rechecking, you are right
Sorry for that
A tentative fix for ADM_Composer::getLastKeyFramePts:
diff --git a/avidemux/common/ADM_editor/src/ADM_edSearch.cpp b/avidemux/common/ADM_editor/src/ADM_edSearch.cpp
index df4385f..51f2837 100644
--- a/avidemux/common/ADM_editor/src/ADM_edSearch.cpp
+++ b/avidemux/common/ADM_editor/src/ADM_edSearch.cpp
@@ -372,12 +372,19 @@ uint64_t ADM_Composer::getLastKeyFramePts(void)
int lastSeg=_segments.getNbSegments();
int seg;
uint64_t pts,dts;
+ int lastRefFrameInSegment;
if(!lastSeg) return ADM_NO_PTS;
for(seg=lastSeg-1;seg>=0;seg--)
{
_SEGMENT *s=_segments.getSegment(seg);
_VIDEOS *v=_segments.getRefVideo(s->_reference);
- int nbFrame=v->_nb_video_frames;
+ uint64_t endTimeInRef=(s->_refStartTimeUs)+(s->_durationUs);
+ if(!getFrameNumFromPtsOrBefore(v,endTimeInRef,lastRefFrameInSegment))
+ {
+ ADM_warning("Cannot map the last frame in segment to reference at PTS=%s\n",ADM_us2plain(endTimeInRef));
+ lastRefFrameInSegment=v->_nb_video_frames;
+ }
+ int nbFrame=lastRefFrameInSegment;
if(!nbFrame) break;
for(int frame=nbFrame-1;frame>=0;frame--)
@@ -388,12 +395,14 @@ uint64_t ADM_Composer::getLastKeyFramePts(void)
if(!(flags & AVI_KEY_FRAME)) continue;
v->_aviheader->getPtsDts(frame,&pts,&dts);
if(pts==ADM_NO_PTS) continue;
- ADM_info("found last keyframe at %d, time=%s\n",frame,ADM_us2plain(pts));
+ ADM_info("found last keyframe at %d, time in reference=%s\n",frame,ADM_us2plain(pts));
break;
}
if(pts!=ADM_NO_PTS)
{
- return pts+s->_startTimeUs-s->_refStartTimeUs;
+ pts+=s->_startTimeUs-s->_refStartTimeUs;
+ ADM_info("found last keyframe at %s\n",ADM_us2plain(pts));
+ return pts;
}
}
ADM_info("Cannot find lastKeyFrame with a valid PTS\n");
The idea is to find out the time in reference corresponding to the end of the last segment first, then find out the frame number at or before that PTS in reference, then search for the last keyframe < this frame as it is done currently. (I'd like to add that I am at a loss about declaring nbFrames as int while everywhere else in the source it is uint32_t, I simply preserved it in the patch)
This fixes deleting to the end of a video and makes ACT_End work for me in all SD mpeg2 videos and all HD h264 videos with a single reference video, but doesn't fix Avidemux hanging for a while for yet unknown reason when hitting the End key with appended HD h264 videos I tested so far (it hangs also without the patch, so it doesn't make things worse).
I've attached the patch for the mentioned workaround without a tailored function for truncating too.
Quote from: eumagga0x2a on October 03, 2016, 02:49:28 PM
This fixes deleting to the end of a video and makes ACT_End work for me in all SD mpeg2 videos and all HD h264 videos with a single reference video, but doesn't fix Avidemux hanging for a while for yet unknown reason when hitting the End key with appended HD h264 videos I tested so far (it hangs also without the patch, so it doesn't make things worse).
FYI: The hang seeking to linear time within the segment representing the appended video is specific to VDPAU decoder. ADM_Composer::switchToSegment fails in ADM_Composer::DecodePictureUpToIntra then.
I've seen that one too
i.e. you decode from 0 to 10 -> all fails, because some ref frames are missing
BUT if you do it again, it will succeed (recovery time ? you have created fake missing ref frames ?)
That trick might not work with vdpau
Some of the patches are a bit kuldgy, so they fit well within avidemux :)
But I'm a bit at loss regarding which ones are needed
(hint toward pull request...)
I'm very sorry for delaying the overhead with GitHub account etc., I have > 10 patches which are very much in flux in my setup and I don't see how this can be managed with pull requests, especially as dealing with my attempts to contribute requires a lot of babysitting on your part anyway.
The both patches in http://avidemux.org/smif/index.php/topic,16935.msg77382.html#msg77382 (http://avidemux.org/smif/index.php/topic,16935.msg77382.html#msg77382) are needed to allow deleting a part of the video including the last frame with the B marker at the last frame.
Thank you very much for your patience.
I finally understood the issue , and yes it was the classic mistake of mixing reference to a thing and the thing itself
First part committed
Thanks!
It must have been painful to narrow that down.
Thank you very much!
Now I'd like to find out if it is necessary to decode anything at the beginning of a segment representing a new reference video if we're looking for a spot much later (e.g. near the end) :)
Depends
Normally, unless there is mix betwen I & IDR, the editor will rewind and decode from the previous intra (even if it is not part of the segment) and then go forward til the actual start of the segment
* edit : clarify
Thank you. I see now that DecodePictureUpToIntra with VDPAU really reliably succeeds on the second try as you explained before.
edit: fits Window not repainted after navigation (http://avidemux.org/smif/index.php/topic,17186.msg77312.html#msg77312) very well.
Maybe it is worth mentioning that every failed navigation or segment switch attempt with VDPAU is accompanied by a
[lavc] Unhandled colorspace: 109 (AV_PIX_FMT_YUV444P10BE=77)
message from ADM_ffmp43.cpp:507 in the console.
Probably an error that is wrongly interpreted
The following patch fixes ADM_Composer::goToTimeVideo with VDPAU and appended videos for me and seems not to cause any side effects:
diff --git a/avidemux/common/ADM_editor/src/ADM_edRender.cpp b/avidemux/common/ADM_editor/src/ADM_edRender.cpp
index 0b29667..5a60f9f 100644
--- a/avidemux/common/ADM_editor/src/ADM_edRender.cpp
+++ b/avidemux/common/ADM_editor/src/ADM_edRender.cpp
@@ -190,8 +190,11 @@ uint32_t seg;
uint64_t to=segTime+s->_refStartTimeUs;
if(false==seektoTime(s->_reference,to))
{
+ if(false==seektoTime(s->_reference,to)) // try twice because the first attempt may fail with VDPAU
+ {
ADM_warning("Cannot seek to beginning of segment %" PRIu32" at %" PRIu64" ms\n",seg,to/1000);
return false;
+ }
}
_currentSegment=seg;
int64_t newTime=(int64_t)v->lastDecodedPts+(int64_t)s->_startTimeUs-(int64_t)s->_refStartTimeUs;
The goToTimeVideo workaround committed as [editor] If seek fails, try a second time. It helps with h26* + hw accel (https://github.com/mean00/avidemux2/commit/889532d7b691717ea6ea78d4275b5e615eea303b), thanks.
WRT other parts of a possible solution or workaround to allow deleting the last frame with marker B at the last frame: Would you like to pursue the path with a custom function (cleaner, but pretty much ado about only a little more than nothing) or the hack resetting the marker B silently to the reported duration of the video?
WRT the reported total duration of the video: I have not looked into this calculation, but ffprobe reports actually the PTS of the last frame as the total duration according to my tests. With total duration beyond the last frame as detected by Avidemux, a workaround is necessary for ACT_GotoMarkB to avoid error when navigating to the B marker upon loading a video. I'd rather start a new topic for this workaround though.
There a general design flaw when the video does not start at zero
It might be worth it to fix for good
Patch welcome :)
1. Are you thinking of making all videos start at zero?
2. Are you thinking of making the PTS of the last frame minus PTS of the first frame be the future total duration of a video?
3. Are these infos from demuxer and decoder correct as of now?
In any case such design flaw is not the reason we can't easily cut off the last frame of video now, but the hackish workaround won't work if (2) is the way to go. (2) will make a workaround for readjusting the B marker obsolete though. With (3) both workarounds are viable and the second is necessary.
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.
Quote from: mean on October 04, 2016, 05:26:27 PM
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 :)
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
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.
I'm not sure if i missed a patch or not ?
I can't say for sure. It was not immediately clear whether http://avidemux.org/smif/index.php/topic,16935.msg77308.html#msg77308 (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 (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
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 (http://avidemux.org/smif/index.php/topic,17194.0.html) and Re: Loading (not appending) a new video resets the encoder settings to "Copy" (http://avidemux.org/smif/index.php/topic,17014.msg77430.html#msg77430) (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.
I think i've taken them all, please say so if i missed something again
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" (http://avidemux.org/smif/index.php/topic,17014.msg77430.html#msg77430). 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.
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
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.:
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
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.
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 ?
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 (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.
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.
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.
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;
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.
The problem with audio is that you dont see them on the main UI
I.e. if it does not do what you want for video, you see it
IF it is the 2nd audio track, you have to go into the audio menu to check if it's ok or not
Similarily, the loaded video might not have audio at all
I agree that there is no immediately obvious solution for audio. The best-effort solution would probably save audio encoder settings only if there is either only a single track or multiple tracks using exactly the same settings and revert to the copy mode if any of these conditions is not met. It would fail gracefully if a video without an audio track is loaded.