Author Topic: Total duration in filter preview after changeFps  (Read 252 times)

eumagga0x2a

  • Hero Member
  • *****
  • Posts: 911
Total duration in filter preview after changeFps
« on: January 05, 2017, 11:42:22 AM »
Would it be possible to find time to investigate the issue

Quote
The total duration accessed as _in->getInfo()->totalDuration is not updated if the changeFps filter changing the duration is added. This might be an issue with underlying code and not the preview dialog's fault.

mentioned in https://github.com/mean00/avidemux2/pull/64 in near future?

Apart from that, it would be immensly helpful if releases were announced a bit in advance, this would save packagers including myself quite a lot of easily avoidable trouble. Thanks!

mean

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 10790
Re: Total duration in filter preview after changeFps
« Reply #1 on: January 05, 2017, 06:20:22 PM »
It has been done in a hurry, the preview bug was really bad
I will not have much time for a few days, so it was now or much later

eumagga0x2a

  • Hero Member
  • *****
  • Posts: 911
Re: Total duration in filter preview after changeFps
« Reply #2 on: January 05, 2017, 07:56:47 PM »
No problem with that, thank you very much for providing solutions that quickly and subsequent speedy bugfix-releases, but it would have been helpful to know that these releases were imminent. In this case I wouldn't have tried to backport >10 post-2.6.15 patches and wouldn't have pushed 2.6.16b to be accepted by RPMFusion for Fedora, then stopping them until the worst issues are fixed upstream, then merging the commits and releasing a patched 2.6.16 for Fedora only to fall one commit short of 2.6.17.

I'll try to look into this issue myself, without any hope to come far, but it will be fun anyway  :)

eumagga0x2a

  • Hero Member
  • *****
  • Posts: 911
Re: Total duration in filter preview after changeFps
« Reply #3 on: January 05, 2017, 08:20:31 PM »
Actually, there is a major issue with 2.6.17 which was not present in 2.6.16 + all later commits except of [filters] Make the preview modal and clean it up, to avoid using stale handles later on: the navigation slider in the common filter preview (the one launched from the video filter manager) doesn't work. Likely related to the cleanup in Q_seekablePreview.{cpp,h}.

eumagga0x2a

  • Hero Member
  • *****
  • Posts: 911
Re: Total duration in filter preview after changeFps
« Reply #4 on: January 05, 2017, 08:58:27 PM »
Yes, reverting the changes to Q_seekablePreview.cpp and Q_seekablePreview.h fixes the navigation slider in the common filter preview window.

mean

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 10790
Re: Total duration in filter preview after changeFps
« Reply #5 on: January 05, 2017, 09:24:56 PM »
I was expecting problems, i hate rushing release like that

So it will be 2.6.18 finally
My last slot is tomorrow morning

mean

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 10790
Re: Total duration in filter preview after changeFps
« Reply #6 on: January 05, 2017, 09:27:41 PM »
The modal change is needed
Without it, try this :

* Add 2 filters (2x flip vertical for example)
* PReview first filter
* Close window
* Preview 2nd filter => slider does not work, nothing works

eumagga0x2a

  • Hero Member
  • *****
  • Posts: 911
Re: Total duration in filter preview after changeFps
« Reply #7 on: January 05, 2017, 09:34:44 PM »
Just the removal of Ui_seekablePreviewWindow::nextImage and Ui_seekablePreviewWindow::sliderChanged causes issues, the modality is fine and necessary.

eumagga0x2a

  • Hero Member
  • *****
  • Posts: 911
Re: Total duration in filter preview after changeFps
« Reply #8 on: January 05, 2017, 10:05:58 PM »
Oh, right, just Ui_seekablePreviewWindow::sliderChanged was required, [Preview] Put back slider signal. Should be managed by flydialog itself fixes the slider. Thanks!

mean

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 10790
Re: Total duration in filter preview after changeFps
« Reply #9 on: January 09, 2017, 07:15:08 AM »
I cannot replicate the issue with change fps

eumagga0x2a

  • Hero Member
  • *****
  • Posts: 911
Re: Total duration in filter preview after changeFps
« Reply #10 on: January 09, 2017, 06:57:08 PM »
You're right, a very poor analysis on my part. Adding debug output showing the value returned by _in->getInfo()->totalDuration proves that it is correct after changeFps. Now for the real issue:

STR:
  • Load a video, select a codec != copy, add changeFps as filter, doubling the FPS of the video to half its duration.
  • From the video filter manager, open the preview.
  • Drag the navigation slider half-way to the right
  • Drag the slider fully to the right

Actual Results:
The very end or a position close to the end of the video is reached after the Step 3, remains unchanged after Step 4.

Expected Results:
The Step 3 navigates to ~50% of the video, the Step 4 to the end.

I'm sorry, I didn't have time to look into the issue and probably won't have much time for Avidemux this week.

eumagga0x2a

  • Hero Member
  • *****
  • Posts: 911
Re: Total duration in filter preview after changeFps
« Reply #11 on: January 09, 2017, 10:01:23 PM »
Oh well, removing the reimplemented goToTime function from changeFps

Code: [Select]
diff --git a/avidemux_plugins/ADM_videoFilters6/changeFps/changeFps.cpp b/avidemux_plugins/ADM_videoFilters6/changeFps/changeFps.cpp
index a12aa93..6b53273 100644
--- a/avidemux_plugins/ADM_videoFilters6/changeFps/changeFps.cpp
+++ b/avidemux_plugins/ADM_videoFilters6/changeFps/changeFps.cpp
@@ -59,7 +59,6 @@ protected:
 public:
                             changeFps(ADM_coreVideoFilter *previous,CONFcouple *conf);
                             ~changeFps();
-        bool                goToTime(uint64_t usSeek);
         virtual const char   *getConfiguration(void);                   /// Return  current configuration as a human readable string
         virtual bool         getNextFrame(uint32_t *fn,ADMImage *image);    /// Return the next image
         virtual bool         getCoupledConf(CONFcouple **couples) ;   /// Return the current filter configuration
@@ -115,21 +114,6 @@ changeFps::~changeFps()
 }
 
 /**
-    \fn goToTime
-    \brief called when seeking. Need to cleanup our stuff.
-*/
-bool         changeFps::goToTime(uint64_t usSeek)
-{
-    double timing=(double)usSeek;
-    timing/=configuration.oldFpsNum;
-    timing/=configuration.newFpsDen;
-    timing*=configuration.newFpsNum;
-    timing*=configuration.oldFpsDen;
-    if(false==ADM_coreVideoFilter::goToTime((uint64_t)timing)) return false;
-    return true;
-}
-
-/**
     \fn getCoupledConf
 */
 bool         changeFps::getCoupledConf(CONFcouple **couples)

would fix the issue in the filter preview, but would require all filters later in the chain using the new timing, which may be inconvenient in some cases (edit: it looks like they have to anyway).
« Last Edit: January 09, 2017, 10:08:16 PM by eumagga0x2a »

eumagga0x2a

  • Hero Member
  • *****
  • Posts: 911
Re: Total duration in filter preview after changeFps
« Reply #12 on: January 12, 2017, 09:45:17 AM »