review done for video thumbnailer (mplayerthumbs)

[Thiago Macieira]

Em Sábado 02 Maio 2009, às 02:38:33, Tom Albers escreveu:
> Op Thursday 23 April 2009 13:03 schreef u:
> > Hi all!
> > With the lastest commit on phonon and mplayerthumbs i implemented phonon
> > support for video thumbnails, enabling then MPlayerThumbs to work using
> > Phonon.
> > Currently the backend selection is configurable with a standalone
> > application (the default is phonon, anyway), together with some advanced
> > parameters.
> > I would propose to move it from kdereview to kdemultimedia, if everybody
> > agrees.
> > Also, i'd think changing the name (just "videothumbnail"?)
>
> Hi,
>
> Nobody on kde-core-devel objected. Feel free to move it (as soon as
> possible please) to kdemultimedia.

Sorry for the delayed objection, but there are grave problems with the code.

MPlayerThumbs is not ready to be out of kdereview. Either revert the move (and 
wait for KDE 4.4) or fix these problems ASAP.

1) it requires a Phonon feature that will not be released with KDE 4.3
  It's breaking the kdemultimedia build. If you want it in kdemultimedia, 
  please disable the feature in mplayerthumb.

2) the Phonon feature it requires only works with the Xine backend
   Since you didn't ask for help in kdemultimedia, I assume you're going to 
   write the code for the other backends too.

This doesn't count as implementation:
QImage VideoWidget::snapshot() const {
  // TODO implement
  return QImage();
}

PS: that also doesn't follow the coding style for the files in question

3) kio_thumbnail crashes wildly with the gstreamer backend
(gdb) bt
#0  0xb3e5a673 in Phonon::VideoWidgetPrivate::setupBackendObject () from 
/home/tmacieir/KDE-4.3/lib/libphonon.so.4
#1  0xb3e5a372 in Phonon::VideoWidget::VideoWidget () from 
/home/tmacieir/KDE-4.3/lib/libphonon.so.4
#2  0xb3e6de7e in PhononBackend::PhononBackend () from 
/home/tmacieir/KDE-4.3/lib/kde4/videopreview.so
#3  0xb3e7742f in ServicesFactory::videoBackend () from 
/home/tmacieir/KDE-4.3/lib/kde4/videopreview.so
#4  0xb3e6ed59 in VideoPreview::create () from 
/home/tmacieir/KDE-4.3/lib/kde4/videopreview.so
#5  0xb7c5a484 in ThumbnailProtocol::get (this=0xbfb13154, url=@0xbfb1356c)
    at 
/home/tmacieir/src/kde4/KDE/kdebase/runtime/kioslave/thumbnail/thumbnail.cpp:275
#6  0xb76fa992 in KIO::SlaveBase::dispatch (this=0xbfb136a4, command=67, 
data=@0xbfb135fc)
    at /home/tmacieir/src/kde4/KDE/kdelibs/kio/kio/slavebase.cpp:1012
#7  0xb76f7282 in KIO::SlaveBase::dispatchLoop (this=0xbfb136a4) at 
/home/tmacieir/src/kde4/KDE/kdelibs/kio/kio/slavebase.cpp:282
#8  0xb7c5b85a in kdemain (argc=4, argv=0x9c37960) at 
/home/tmacieir/src/kde4/KDE/kdebase/runtime/kioslave/thumbnail/thumbnail.cpp:136
#9  0x0804cb2e in launch (argc=4, _name=0x9c3c35c "kio_thumbnail", 
args=0x9c3c3e4 "", cwd=0x0, envc=0, envs=0x9c3c3e8 "", reset_env=false,
    tty=0x0, avoid_loops=false, startup_id_str=0x8051607 "0") at 
/home/tmacieir/src/kde4/KDE/kdelibs/kinit/kinit.cpp:654
#10 0x0804dbb0 in handle_launcher_request (sock=7, who=0x80518a0 "launcher") 
at /home/tmacieir/src/kde4/KDE/kdelibs/kinit/kinit.cpp:1146
#11 0x0804e2d5 in handle_requests (waitForPid=0) at 
/home/tmacieir/src/kde4/KDE/kdelibs/kinit/kinit.cpp:1339
#12 0x0804f6e6 in main (argc=1, argv=0xbfb13d24, envp=0xbfb13d2c) at 
/home/tmacieir/src/kde4/KDE/kdelibs/kinit/kinit.cpp:1766


-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
  Senior Product Manager - Nokia, Qt Software
      PGP/GPG: 0x6EF45358; fingerprint:
      E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20090507/2382480e/attachment.sig>