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

[yikei lu]

Well then please ignore this patch :)

2016-04-12 22:13 GMT+09:00 Hugo Beauzée-Luyssen <hugo at beauzee.fr>:
> 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.
>
> _______________________________________________
> Vlmc-devel mailing list
> Vlmc-devel at videolan.org
> https://mailman.videolan.org/listinfo/vlmc-devel