[vlmc-devel] [PATCH 4/4] VLCSource: Avoid deleting VmemRenderer too early

[Hugo Beauzée-Luyssen]

On 04/12/2016 02:56 PM, yikei lu wrote:
> yes, if I don't apply this patch, I'll get only a half of a snapshot
>
> 2016-04-12 21:51 GMT+09:00 Hugo Beauzée-Luyssen <hugo at beauzee.fr>:
>> On 04/12/2016 01:10 PM, Yikai Lu wrote:
>>>
>>> If we declare in a function like "VmemRenderer renderer( some values )"
>>> just as we did before, it will be removed as soon as the function ends.
>>> Since the renderer don't know how much time should be needed to render a
>>> snapshot, we should keep it alive as long as possible.
>>> ---
>>>    src/Backend/VLC/VLCSource.cpp | 20 +++++++++++++-------
>>>    src/Backend/VLC/VLCSource.h   |  4 +++-
>>>    2 files changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/Backend/VLC/VLCSource.cpp b/src/Backend/VLC/VLCSource.cpp
>>> index d38d12f..3257781 100644
>>> --- a/src/Backend/VLC/VLCSource.cpp
>>> +++ b/src/Backend/VLC/VLCSource.cpp
>>> @@ -39,10 +39,16 @@ VLCSource::VLCSource( VLCBackend* backend, const
>>> QString& path )
>>>        , m_snapshot( nullptr )
>>>        , m_isParsed( false )
>>>        , m_nbFrames( 0 )
>>> +    , m_vmemRenderer( nullptr )
>>>    {
>>>        m_media = ::VLC::Media( backend->vlcInstance(), path.toStdString(),
>>> ::VLC::Media::FromPath );
>>>    }
>>>
>>> +VLCSource::~VLCSource()
>>> +{
>>> +    delete m_vmemRenderer;
>>> +}
>>> +
>>>    ::VLC::Media&
>>>    VLCSource::media()
>>>    {
>>> @@ -63,7 +69,6 @@ VLCSource::preparse()
>>>        Q_ASSERT( m_nbAudioTracks == 0 );
>>>        Q_ASSERT( m_nbVideoTracks == 0 );
>>>
>>> -    VmemRenderer renderer( m_backend, this, nullptr );
>>>        m_media.parse();
>>>        m_length = m_media.duration();
>>>        auto tracks = m_media.tracks();
>>> @@ -85,7 +90,7 @@ VLCSource::preparse()
>>>                    m_width = t.width();
>>>                    m_height = t.height();
>>>                    m_nbFrames = (int64_t)( (float)( m_length / 1000 ) *
>>> m_fps );
>>> -                computeSnapshot( renderer );
>>> +                computeSnapshot();
>>>                }
>>>            }
>>>            else if ( t.type() == ::VLC::MediaTrack::Type::Audio )
>>> @@ -102,25 +107,26 @@ VLCSource::isParsed() const
>>>    }
>>>
>>>    bool
>>> -VLCSource::computeSnapshot( VmemRenderer& renderer )
>>> +VLCSource::computeSnapshot()
>>>    {
>>>        Q_ASSERT( m_snapshot == nullptr );
>>> -    renderer.start();
>>> +    m_vmemRenderer = new VmemRenderer( m_backend, this, nullptr );
>>> +    m_vmemRenderer->start();
>>>        {
>>>            QMutex mutex;
>>>            QWaitCondition cond;
>>> -        auto em = renderer.mediaPlayer().eventManager();
>>> +        auto em = m_vmemRenderer->mediaPlayer().eventManager();
>>>            em.onPositionChanged([&mutex, &cond](float pos) {
>>>                QMutexLocker lock( &mutex );
>>>                if ( pos > 0.2 )
>>>                    cond.wakeAll();
>>>            });
>>>            QMutexLocker lock( &mutex );
>>> -        renderer.setPosition( 0.3 );
>>> +        m_vmemRenderer->setPosition( 0.3 );
>>>            if ( cond.wait( &mutex, 2000 ) == false )
>>>                return false;
>>>        }
>>> -    m_snapshot = renderer.waitSnapshot();
>>> +    m_snapshot = m_vmemRenderer->waitSnapshot();
>>>        return m_snapshot != nullptr;
>>>    }
>>>
>>> diff --git a/src/Backend/VLC/VLCSource.h b/src/Backend/VLC/VLCSource.h
>>> index 10a7d08..346adbe 100644
>>> --- a/src/Backend/VLC/VLCSource.h
>>> +++ b/src/Backend/VLC/VLCSource.h
>>> @@ -40,6 +40,7 @@ class VLCSource : public ISource
>>>    {
>>>    public:
>>>        VLCSource( VLCBackend* backend, const QString& path );
>>> +    virtual ~VLCSource();
>>>        virtual ISourceRenderer*    createRenderer( ISourceRendererEventCb*
>>> callback );
>>>        virtual bool                preparse();
>>>        virtual bool                isParsed() const;
>>> @@ -58,7 +59,7 @@ public:
>>>        ::VLC::Media&               media();
>>>
>>>    private:
>>> -    bool                        computeSnapshot( VmemRenderer& renderer
>>> );
>>> +    bool                        computeSnapshot();
>>>
>>>    private:
>>>        VLCBackend*                 m_backend;
>>> @@ -72,6 +73,7 @@ private:
>>>        uint8_t*                    m_snapshot;
>>>        bool                        m_isParsed;
>>>        int64_t                     m_nbFrames;
>>> +    VmemRenderer*               m_vmemRenderer;
>>>    };
>>>
>>>
>>>
>> The renderer will stop the VLC::MediaPlayer upon destruction, so that
>> shouldn't be a problem. Moreover, the waitSnapshot method ensure we block
>> until the snapshot is computed, so the timing shouldn't be an issue.
>> Did you encounter an issue related to this code?
>> _______________________________________________
>> Vlmc-devel mailing list
>> Vlmc-devel at videolan.org
>> https://mailman.videolan.org/listinfo/vlmc-devel
> _______________________________________________
> Vlmc-devel mailing list
> Vlmc-devel at videolan.org
> https://mailman.videolan.org/listinfo/vlmc-devel
>
This is weird.
What bothers me is that by doing so you don't stop the mediaplayer.
There could be a bug in the waitSnapshot method which causes it to 
return too early, however the only thing I can think of that causes half 
a snapshot to be rendered, is if the vmem is rendering into the frame 
while we are using it to compute the media's QImage.
That should be garanteed by the current code, as the media player is 
stopped as soon as we leave the scope containing the VmemRenderer. You 
are right in moving the VmemRenderer into the computeSnapshot method, 
however your changes seems more racy than the old version, so I'm a bit 
confused as of why results are better on your end.
And again, this code has been implemented (hopefully in a better way) in 
the new media library, so maybe it's not worth focusing on it.