[vlmc-devel] [PATCH] Clip: compute length relying on ISource length()

[Paweł Goliński]

1. Seconds are used for length computation and display I think (in media library widget). I didn’t research where else this value is used. Will change the code as requested in next version of the patch.
2. IIRC removing m_begin and m_end may require a non-trivial refactor of dependent code. m_begin and m_end give us EXACT frames and when we use m_beginPosition and m_endPosition only, frame computation may lose some precision which may lead to some bugs in dependent code.

> Wiadomość napisana przez Hugo Beauzée-Luyssen <hugo at beauzee.fr> w dniu 09.05.2016, o godz. 11:36:
> 
> On 05/02/2016 08:20 PM, Pawel Golinski wrote:
>> Length was computed using frames only (and ISource's length() was
>> ignored) which caused it to be zero in sound files.
>> ---
>>  src/Media/Clip.cpp | 16 +++++++++++-----
>>  src/Media/Clip.h   |  8 ++++++++
>>  2 files changed, 19 insertions(+), 5 deletions(-)
>> 
>> diff --git a/src/Media/Clip.cpp b/src/Media/Clip.cpp
>> index 4f29f8e..b634442 100644
>> --- a/src/Media/Clip.cpp
>> +++ b/src/Media/Clip.cpp
>> @@ -40,12 +40,15 @@ Clip::Clip( Media *media, qint64 begin /*= 0*/, qint64 end /*= -1*/, const QStri
>>          m_end( end ),
>>          m_parent( media->baseClip() )
>>  {
>> +    int64_t nbSourceFrames = media->source()->nbFrames();
>>      if ( end == -1 )
>> -        m_end = media->source()->nbFrames();
>> +        m_end = nbSourceFrames;
>>      if ( uuid.isEmpty() == true )
>>          m_uuid = QUuid::createUuid();
>>      else
>>          m_uuid = QUuid( uuid );
>> +    m_beginPosition = (float)begin / (float)nbSourceFrames;
>> +    m_endPosition = (float)end / (float)nbSourceFrames;
>>      m_childs = new MediaContainer( this );
>>      m_rootClip = media->baseClip();
>>      computeLength();
>> @@ -61,6 +64,7 @@ Clip::Clip( Clip *parent, qint64 begin /*= -1*/, qint64 end /*= -1*/,
>>          m_rootClip( parent->rootClip() ),
>>          m_parent( parent )
>>  {
>> +    int64_t nbSourceFrames = parent->media()->source()->nbFrames();
>>      if ( begin < 0 )
>>          m_begin = parent->m_begin;
>>      if ( end < 0 )
>> @@ -69,6 +73,8 @@ Clip::Clip( Clip *parent, qint64 begin /*= -1*/, qint64 end /*= -1*/,
>>          m_uuid = QUuid::createUuid();
>>      else
>>          m_uuid = QUuid( uuid );
>> +    m_beginPosition = (float)begin / (float)nbSourceFrames;
>> +    m_endPosition = (float)end / (float)nbSourceFrames;
>>      m_childs = new MediaContainer( this );
>>      computeLength();
>>  }
>> @@ -108,11 +114,9 @@ Clip::lengthSecond() const
>>  void
>>  Clip::computeLength()
>>  {
>> -    float   fps = m_media->source()->fps();
>> -    if ( fps < 0.1f )
>> -        fps = Clip::DefaultFPS;
>> +    int64_t sourceLengthSeconds = m_media->source()->length() / 1000;
>>      m_nbFrames = m_end - m_begin;
>> -    m_lengthSeconds = qRound64( (float)m_nbFrames / fps );
>> +    m_lengthSeconds = qRound64( ( m_endPosition - m_beginPosition ) * sourceLengthSeconds );
> 
> It would probably allow for a better precision to use milliseconds first, and then convert to seconds.
> Though it feels strange to use seconds anyway, but I don't recall which unit gets used when :/
> 
>>  }
>> 
>>  const QStringList&
>> @@ -274,6 +278,8 @@ Clip::mediaMetadataUpdated()
>>      {
>>          m_begin = 0;
>>          m_end = m_media->source()->nbFrames();
>> +        m_beginPosition = 0.0f;
>> +        m_endPosition = 1.0f;
>>          computeLength();
>>      }
>>  }
>> diff --git a/src/Media/Clip.h b/src/Media/Clip.h
>> index 5fb5997..8b49b9a 100644
>> --- a/src/Media/Clip.h
>> +++ b/src/Media/Clip.h
>> @@ -149,6 +149,14 @@ class   Clip : public QObject
>>           *          beginning of the parent Media.
>>           */
>>          qint64              m_end;
>> +        /**
>> +         *  \brief  This represents the beginning of the Clip in form of [0; 1] float
>> +         */
>> +        float              m_beginPosition;
>> +        /**
>> +         *  \brief  This represents the end of the Clip in form of [0;1] float
>> +         */
>> +        float              m_endPosition;
>> 
>>          /**
>>           *  \brief  The length in frames
>> 
> 
> Do we need to keep m_begin & m_end if we switch to a position based approach?
> _______________________________________________
> Vlmc-devel mailing list
> Vlmc-devel at videolan.org <mailto:Vlmc-devel at videolan.org>
> https://mailman.videolan.org/listinfo/vlmc-devel <https://mailman.videolan.org/listinfo/vlmc-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlmc-devel/attachments/20160509/eccc1f0c/attachment-0001.html>