Newbie question- What is the proper way to add a clip to an existing file?

Started by Accomac, September 16, 2016, 03:35:00 PM

Previous topic - Next topic

eumagga0x2a

Quote from: eumagga0x2a on September 18, 2016, 01:15:24 PM
Naively deleting

&& (s._startTimeUs+s._durationUs)>endTime

and replacing the line 829 with

            uint64_t offset=endTime-startTime;

sort of fixes the copy to clipboard function.

I'm sorry for very incomplete analysis and wrong approach. There was nothing wrong with the offset calculation, but I'm pretty sure that both the second condition and the calculation of s._durationUs in this context was incorrect.

diff --git a/avidemux/common/ADM_editor/src/ADM_segment.cpp b/avidemux/common/ADM_editor/src/ADM_segment.cpp
index ef4791d..f85ab05 100644
--- a/avidemux/common/ADM_editor/src/ADM_segment.cpp
+++ b/avidemux/common/ADM_editor/src/ADM_segment.cpp
@@ -822,12 +822,12 @@ bool        ADM_EditorSegment::copyToClipBoard(uint64_t startTime, uint64_t endT
             s._durationUs-=offset;         // take into account the part we chopped
             aprintf("Marker A is here offset=%d\n",(int)offset);
         }
-        if(s._startTimeUs<=endTime && (s._startTimeUs+s._durationUs)>endTime)
+        if(s._startTimeUs<=endTime && (startTime+s._durationUs)>endTime)
         {
             
             // need to refine last seg           
             uint64_t offset=endTime-s._startTimeUs;
-            s._durationUs=offset;
+            s._durationUs=endTime-startTime;
             aprintf("Marker B is here offset=%d\n",(int)offset);
         }
         // TODO refine timing for 1st/last/duration/...


fixes the copy to clipboard function for me and doesn't kill kittens as it seems.

eumagga0x2a

Quote from: Accomac on September 19, 2016, 04:10:26 PM
Whew!

It was never my intention to start an argument among everyone, I just wanted to know what I was doing wrong when using your program.

There was no argument except of a minor misunderstanding. Thank you very much for attracting attention to this bug, which I saw myself a month or two ago, wasn't able to dissect and finally just forgot about it.

QuoteI have never ignored anyone's posting or suggestion with a solution or workaround. When I get a reply from a senior member 'eumagga0x2a' :You don't do anything wrong, it is a bug. Currently it seems that the position of the B marker is not taken into account when copying video. "Paste" pastes everything from the A marker till the end of the video at the current slider position. I have to believe them!

No, you have not, I'm not a deity :)

Anyway, if you build Avidemux on Mint yourself and want copy to clipboard function to work immediately, you could give the patch a try. Otherwise I'm sure that Mean will fix the issue in a proper way pretty soon. You will be able to obtain Avidemux nightly deb packages thanks to Jan's efforts then.

Accomac

hello again eumagga0x2a,

I should have mentioned earlier that I had a lot of trouble finding  a version of v2.6.13 that my Norton Security didn't think was a virus or malware. I don't know if the suspect versions were really infected or it was a false positive from Norton.

I have been trying the workarounds you all have suggested to me as far as making additions to an original video. I have had success doing it that way IE: clip a goes to clip b etc. but of course that way is very time consuming depending on the amount of edits I want to do.

Once the bug issue is resolved by mean, yourself and the others, will I then be able to use the [A & B] to cut and paste a clip into an existing file? If I read things correctly that's the goal.

Anyway I can work with what I have now and look forward to the 'fix' whenever it comes, there's no hurry. I would think that open source programming is a thankless endeavor, so let me say 'THANK YOU' to all of you that created Avidemux! Your work isn't going un-appreciated, I think people just forget to say those two words.

Regards,

Accomac


eumagga0x2a

Quote from: Accomac on September 20, 2016, 04:40:15 PM
I should have mentioned earlier that I had a lot of trouble finding  a version of v2.6.13 that my Norton Security didn't think was a virus or malware. I don't know if the suspect versions were really infected or it was a false positive from Norton.

You should always check at least published MD5 checksums for release executables you download. If they are not signed, a quick search in Google for the particular checksum allows somewhat to estimate the likelihood that this checksum is valid (yes, md5 hashing is already cracked, but there is nothing better for Avidemux yet). If the checksum matches, it was probably a false positive. If not, everything is possible, from a damaged file till a malitious attack.

You mentioned that you use Linux Mint alongside with Windows. You should not have any such problems on Linux.

Quote from: mean on September 20, 2016, 05:07:55 PM
Maybe better now

Unfortunately, not quite. s._durationUs still ends up being equal endTime. I witnessed also Avidemux not reproducibly crashing on Ctrl+C without any backtrace.

eumagga0x2a

The following works for me, tested with quite complicated multi-segment layouts:

diff --git a/avidemux/common/ADM_editor/src/ADM_segment.cpp b/avidemux/common/ADM_editor/src/ADM_segment.cpp
index 090d6b7..89b731f 100644
--- a/avidemux/common/ADM_editor/src/ADM_segment.cpp
+++ b/avidemux/common/ADM_editor/src/ADM_segment.cpp
@@ -826,7 +826,7 @@ bool        ADM_EditorSegment::copyToClipBoard(uint64_t startTime, uint64_t endT
         {           
             // need to refine last seg           
             uint64_t offset=endTime-s._startTimeUs;
-            s._durationUs=offset;
+            s._durationUs=endTime-startTime;
             aprintf("Marker B is here offset=%d\n",(int)offset);
         }
         // TODO refine timing for 1st/last/duration/...


(If it says that "Marker B" is equal offset, how can the duration be equal offset as well??

mean

Let' say you have 3 segments
0: 0..5 duration=5 Startime =0 endTime=5
1: 10..15 duration = 5 Startime =5 endTime=10
2: 40..45 duration = 5 Startime =10 endTime=15

if you copy to clip board from middle of 1 to middle of 2, i.e. 5 seconddes
you will have half of 1 from 12.5 to 15 i.e. 7.5 to 10 linear, duration =2.5
but you must  also have  half of 2 duration = 2.5 from 40 to 42.5, linear 10 to 12.5
if you do end segment  duration = startime-endtime you will get 5
Here we do
duration = end time (12.5) - segment start time = 10 == 2.5

What am i missing ?


eumagga0x2a

Thank you for your patient explanation, now I understand what a segment is. The current code works correctly only if mark A and mark B belong to different segments. My otherwise wrong suggestion works only if A and B belong to one and the same segment. What do you think about the following approach?

diff --git a/avidemux/common/ADM_editor/src/ADM_segment.cpp b/avidemux/common/ADM_editor/src/ADM_segment.cpp
index 090d6b7..c3d9807 100644
--- a/avidemux/common/ADM_editor/src/ADM_segment.cpp
+++ b/avidemux/common/ADM_editor/src/ADM_segment.cpp
@@ -813,21 +813,23 @@ bool        ADM_EditorSegment::copyToClipBoard(uint64_t startTime, uint64_t endT
     {
         _SEGMENT s=segments[seg],s2=s;
         aprintf("Adding segment %d to clipboard\n",seg);
+        uint64_t offsetA = 0;
+        uint64_t offsetB = 0;
         if(s._startTimeUs<=startTime && (s._startTimeUs+s._durationUs)>startTime)
         {
             // need to refine 1st seg

-            uint64_t offset=startTime-s._startTimeUs;
-            s._refStartTimeUs+=offset;
-            s._durationUs-=offset;         // take into account the part we chopped
-            aprintf("Marker A is here offset=%d\n",(int)offset);
+            offsetA=startTime-s._startTimeUs;
+            s._refStartTimeUs+=offsetA;
+            s._durationUs-=offsetA;         // take into account the part we chopped
+            aprintf("Marker A is here offset=%d\n",(int)offsetA);
         }
         if(s2._startTimeUs<=endTime && (s2._startTimeUs+s2._durationUs)>=endTime)
         {           
             // need to refine last seg           
-            uint64_t offset=endTime-s._startTimeUs;
-            s._durationUs=offset;
-            aprintf("Marker B is here offset=%d\n",(int)offset);
+            offsetB=endTime-s._startTimeUs;
+            s._durationUs=offsetB-offsetA;
+            aprintf("Marker B is here offset=%d\n",(int)offsetB);
         }
         // TODO refine timing for 1st/last/duration/...
         clipboard.push_back(s);       


It seems to cover both scenarios.

eumagga0x2a

Quote from: eumagga0x2a on September 20, 2016, 09:16:24 PM
The current code works correctly only if mark A and mark B belong to different segments.

Assuming that there are two segments




0:0...5duration=5start_time=0end_time=5
1:10...20duration=10start_time=5end_time=15

Now copying to clipboard from linear 8 to 13 results at line 820 in offset=8-5=3. At the line 822 duration becomes 10-3=7. The second test evaluates after the recent partial fix correctly as true too, so we get a new offset=13-5=8 and a new duration=8 instead. Finally we try to copy from linear 8 to linear 16 which is beyond the end of the video.

The new patch would result in offsetA=8-5=3 as before, but then we get offsetB=8 and the correct duration of offsetB-offsetA=8-3=5.

In your example with 3 segments, the offsetA for segment 1 becomes 7.5-5=2.5 and duration=5-2.5=2.5 while the second test evaluates false. For segment 2, the first test evaluates false, so offsetA is zero. offsetB is 12.5-10=2.5 and duration=offsetB-offsetA=2.5-0=2.5. We end up copying 2.5 seconds starting at linear 7.5 from the segment 1 and 2.5 seconds starting at linear 10 ending at 12.5 from the segment 2, which is the expected correct behaviour.

mean

It looks fine, but a bit complicated
Wouldn't just adding
// First seg
{
...

s._startTimeUs+=offset
}
// last seg
{

}
in the first part have the same effect but simpler ?

eumagga0x2a

It would work but print wrong value for marker B offset if both markers lie within one and the same segment.

(For the sake of easy legibility I would rather not modify the value of a variable which would change the semantics, i.e. _startTimeUs stores the start time of the segment and doesn't morph into something else. If you don't mind...)

mean


It is a copy of the real one, that copy is stored in the clipboard .
In that case, that specific field has no meaning because it needs the previous segments to be computed correctly.
That field is  erased and recomputed  when the clipboard is pasted , where it has a context

eumagga0x2a

Then

diff --git a/avidemux/common/ADM_editor/src/ADM_segment.cpp b/avidemux/common/ADM_editor/src/ADM_segment.cpp
index 090d6b7..2209c88 100644
--- a/avidemux/common/ADM_editor/src/ADM_segment.cpp
+++ b/avidemux/common/ADM_editor/src/ADM_segment.cpp
@@ -820,13 +820,14 @@ bool        ADM_EditorSegment::copyToClipBoard(uint64_t startTime, uint64_t endT
             uint64_t offset=startTime-s._startTimeUs;
             s._refStartTimeUs+=offset;
             s._durationUs-=offset;         // take into account the part we chopped
+            s._startTimeUs+=offset;
             aprintf("Marker A is here offset=%d\n",(int)offset);
         }
         if(s2._startTimeUs<=endTime && (s2._startTimeUs+s2._durationUs)>=endTime)
         {           
             // need to refine last seg           
-            uint64_t offset=endTime-s._startTimeUs;
-            s._durationUs=offset;
+            uint64_t offset=endTime-s2._startTimeUs;
+            s._durationUs=endTime-s._startTimeUs;
             aprintf("Marker B is here offset=%d\n",(int)offset);
         }
         // TODO refine timing for 1st/last/duration/...


would probably do it (untested).


Accomac

Hello again,

I didn't want to start a new thread since the problem is the same.

Using the latest version of Avidemux and I find that the problem with the A[ copying to the end of the video is still there. It blows right past the ]B as if it wasn't there.

Not sure if that was one of the things that was to be fixed in this release or not, but I thought I'd just mention it.

Thanks to you all,

Accomac