Codereview comments from r637

This commit is contained in:
David Sansome 2010-04-12 00:20:52 +00:00
parent 031752823c
commit 1b0b8979df
4 changed files with 49 additions and 39 deletions

View File

@ -68,7 +68,7 @@ GstEngine::~GstEngine() {
gst_object_unref(GST_OBJECT(can_decode_pipeline_)); gst_object_unref(GST_OBJECT(can_decode_pipeline_));
// Destroy scope delay queue // Destroy scope delay queue
ClearScopeQ(); ClearScopeBuffers();
g_queue_free(delayq_); g_queue_free(delayq_);
// Save configuration // Save configuration
@ -147,7 +147,7 @@ bool GstEngine::CanDecode(const QUrl &url) {
} }
void GstEngine::CanDecodeNewPadCallback(GstElement*, GstPad* pad, gboolean, gpointer self) { void GstEngine::CanDecodeNewPadCallback(GstElement*, GstPad* pad, gboolean, gpointer self) {
GstEngine* instance = static_cast<GstEngine*>(self); GstEngine* instance = reinterpret_cast<GstEngine*>(self);
GstCaps* caps = gst_pad_get_caps(pad); GstCaps* caps = gst_pad_get_caps(pad);
if (gst_caps_get_size(caps) > 0) { if (gst_caps_get_size(caps) > 0) {
@ -159,7 +159,7 @@ void GstEngine::CanDecodeNewPadCallback(GstElement*, GstPad* pad, gboolean, gpoi
} }
void GstEngine::CanDecodeLastCallback(GstElement*, gpointer self) { void GstEngine::CanDecodeLastCallback(GstElement*, gpointer self) {
GstEngine* instance = static_cast<GstEngine*>(self); GstEngine* instance = reinterpret_cast<GstEngine*>(self);
instance->can_decode_last_ = true; instance->can_decode_last_ = true;
} }
@ -192,7 +192,7 @@ Engine::State GstEngine::state() const {
} }
} }
void GstEngine::NewBuffer(GstBuffer* buf) { void GstEngine::AddBufferToScope(GstBuffer* buf) {
g_queue_push_tail(delayq_, buf); g_queue_push_tail(delayq_, buf);
} }
@ -316,7 +316,7 @@ bool GstEngine::Load(const QUrl& url) {
void GstEngine::StartFadeout() { void GstEngine::StartFadeout() {
fadeout_pipeline_ = current_pipeline_; fadeout_pipeline_ = current_pipeline_;
disconnect(fadeout_pipeline_.get(), 0, 0, 0); disconnect(fadeout_pipeline_.get(), 0, 0, 0);
ClearScopeQ(); ClearScopeBuffers();
fadeout_pipeline_->StartFader(fadeout_duration_, QTimeLine::Backward); fadeout_pipeline_->StartFader(fadeout_duration_, QTimeLine::Backward);
connect(fadeout_pipeline_.get(), SIGNAL(FaderFinished()), SLOT(FadeoutFinished())); connect(fadeout_pipeline_.get(), SIGNAL(FaderFinished()), SLOT(FadeoutFinished()));
@ -380,7 +380,7 @@ void GstEngine::Seek(uint ms) {
return; return;
if (current_pipeline_->Seek(ms * GST_MSECOND)) if (current_pipeline_->Seek(ms * GST_MSECOND))
ClearScopeQ(); ClearScopeBuffers();
else else
qDebug() << "Seek failed"; qDebug() << "Seek failed";
} }
@ -426,7 +426,7 @@ void GstEngine::timerEvent( QTimerEvent* ) {
} }
void GstEngine::HandlePipelineError(const QString& message) { void GstEngine::HandlePipelineError(const QString& message) {
qDebug() << "Gstreamer error:" << message; qWarning() << "Gstreamer error:" << message;
current_pipeline_.reset(); current_pipeline_.reset();
} }
@ -493,11 +493,11 @@ shared_ptr<GstEnginePipeline> GstEngine::CreatePipeline(const QUrl& url) {
ret->set_output_device(sink_, device_); ret->set_output_device(sink_, device_);
connect(ret.get(), SIGNAL(EndOfStreamReached()), SLOT(EndOfStreamReached())); connect(ret.get(), SIGNAL(EndOfStreamReached()), SLOT(EndOfStreamReached()));
connect(ret.get(), SIGNAL(BufferFound(GstBuffer*)), SLOT(NewBuffer(GstBuffer*))); connect(ret.get(), SIGNAL(BufferFound(GstBuffer*)), SLOT(AddBufferToScope(GstBuffer*)));
connect(ret.get(), SIGNAL(Error(QString)), SLOT(HandlePipelineError(QString))); connect(ret.get(), SIGNAL(Error(QString)), SLOT(HandlePipelineError(QString)));
connect(ret.get(), SIGNAL(MetadataFound(Engine::SimpleMetaBundle)), connect(ret.get(), SIGNAL(MetadataFound(Engine::SimpleMetaBundle)),
SLOT(NewMetaData(Engine::SimpleMetaBundle))); SLOT(NewMetaData(Engine::SimpleMetaBundle)));
connect(ret.get(), SIGNAL(destroyed()), SLOT(ClearScopeQ())); connect(ret.get(), SIGNAL(destroyed()), SLOT(ClearScopeBuffers()));
if (!ret->Init(url)) if (!ret->Init(url))
ret.reset(); ret.reset();
@ -510,7 +510,7 @@ qint64 GstEngine::PruneScope() {
return 0; return 0;
// get the position playing in the audio device // get the position playing in the audio device
gint64 pos = current_pipeline_->position(); qint64 pos = current_pipeline_->position();
GstBuffer *buf = 0; GstBuffer *buf = 0;
quint64 etime; quint64 etime;
@ -538,7 +538,7 @@ qint64 GstEngine::PruneScope() {
return pos; return pos;
} }
void GstEngine::ClearScopeQ() { void GstEngine::ClearScopeBuffers() {
// just free them all // just free them all
while (g_queue_get_length(delayq_)) { while (g_queue_get_length(delayq_)) {
GstBuffer* buf = reinterpret_cast<GstBuffer *>( g_queue_pop_head(delayq_) ); GstBuffer* buf = reinterpret_cast<GstBuffer *>( g_queue_pop_head(delayq_) );

View File

@ -99,8 +99,8 @@ class GstEngine : public Engine::Base {
void EndOfStreamReached(); void EndOfStreamReached();
void HandlePipelineError(const QString& message); void HandlePipelineError(const QString& message);
void NewMetaData(const Engine::SimpleMetaBundle& bundle); void NewMetaData(const Engine::SimpleMetaBundle& bundle);
void NewBuffer(GstBuffer* buf); void AddBufferToScope(GstBuffer* buf);
void ClearScopeQ(); void ClearScopeBuffers();
void FadeoutFinished(); void FadeoutFinished();
private: private:

View File

@ -48,6 +48,15 @@ void GstEnginePipeline::set_output_device(const QString &sink, const QString &de
bool GstEnginePipeline::Init(const QUrl &url) { bool GstEnginePipeline::Init(const QUrl &url) {
pipeline_ = gst_pipeline_new("pipeline"); pipeline_ = gst_pipeline_new("pipeline");
// Here we create all the parts of the gstreamer pipeline - from the source
// to the sink. The parts of the pipeline are split up into bins:
// source -> decode bin -> audio bin
// The decode bin is a gstreamer builtin that automatically picks the right
// decoder for the file.
// The audio bin gets created here and contains:
// audioconvert -> equalizer -> volume -> audioscale -> audioconvert ->
// audiosink
// Source // Source
src_ = GstEngine::CreateElement("giosrc"); src_ = GstEngine::CreateElement("giosrc");
if (!src_) if (!src_)
@ -60,13 +69,14 @@ bool GstEnginePipeline::Init(const QUrl &url) {
if (!(decodebin_ = GstEngine::CreateElement("decodebin", pipeline_))) { return false; } if (!(decodebin_ = GstEngine::CreateElement("decodebin", pipeline_))) { return false; }
g_signal_connect(G_OBJECT(decodebin_), "new-decoded-pad", G_CALLBACK(NewPadCallback), this); g_signal_connect(G_OBJECT(decodebin_), "new-decoded-pad", G_CALLBACK(NewPadCallback), this);
// Does some stuff with ghost pads
GstPad* pad = gst_element_get_pad(decodebin_, "sink"); GstPad* pad = gst_element_get_pad(decodebin_, "sink");
if (pad) { if (pad) {
event_cb_id_ = gst_pad_add_event_probe (pad, G_CALLBACK(EventCallback), this); event_cb_id_ = gst_pad_add_event_probe (pad, G_CALLBACK(EventCallback), this);
gst_object_unref(pad); gst_object_unref(pad);
} }
// The link from decodebin to audioconvert will be made in the newPad-callback // The link from decodebin to audioconvert will be made in NewPadCallback
gst_element_link(src_, decodebin_); gst_element_link(src_, decodebin_);
// Audio bin // Audio bin
@ -90,8 +100,8 @@ bool GstEnginePipeline::Init(const QUrl &url) {
gst_element_add_pad(audiobin_, gst_ghost_pad_new("sink", pad)); gst_element_add_pad(audiobin_, gst_ghost_pad_new("sink", pad));
gst_object_unref(pad); gst_object_unref(pad);
// add a data probe on the src pad if the audioconvert element for our scope // Add a data probe on the src pad of the audioconvert element for our scope.
// we do it here because we want pre-equalized and pre-volume samples // We do it here because we want pre-equalized and pre-volume samples
// so that our visualization are not affected by them // so that our visualization are not affected by them
pad = gst_element_get_pad(audioconvert_, "src"); pad = gst_element_get_pad(audioconvert_, "src");
gst_pad_add_buffer_probe(pad, G_CALLBACK(HandoffCallback), this); gst_pad_add_buffer_probe(pad, G_CALLBACK(HandoffCallback), this);
@ -134,7 +144,7 @@ GstEnginePipeline::~GstEnginePipeline() {
gboolean GstEnginePipeline::BusCallback(GstBus*, GstMessage* msg, gpointer self) { gboolean GstEnginePipeline::BusCallback(GstBus*, GstMessage* msg, gpointer self) {
GstEnginePipeline* instance = static_cast<GstEnginePipeline*>(self); GstEnginePipeline* instance = reinterpret_cast<GstEnginePipeline*>(self);
switch ( GST_MESSAGE_TYPE(msg)) { switch ( GST_MESSAGE_TYPE(msg)) {
case GST_MESSAGE_ERROR: { case GST_MESSAGE_ERROR: {
@ -142,40 +152,40 @@ gboolean GstEnginePipeline::BusCallback(GstBus*, GstMessage* msg, gpointer self)
gchar* debugs; gchar* debugs;
gst_message_parse_error(msg, &error, &debugs); gst_message_parse_error(msg, &error, &debugs);
qDebug() << "ERROR RECEIVED IN BUS_CB <" << error->message << ">" ; qWarning() << "ERROR RECEIVED IN BUS_CB <" << error->message << ">" ;
emit instance->Error(QString::fromAscii(error->message)); emit instance->Error(QString::fromAscii(error->message));
break; break;
} }
case GST_MESSAGE_TAG: { case GST_MESSAGE_TAG: {
gchar* string=NULL; gchar* data = NULL;
Engine::SimpleMetaBundle bundle; Engine::SimpleMetaBundle bundle;
GstTagList* taglist; GstTagList* taglist;
gst_message_parse_tag(msg,&taglist); gst_message_parse_tag(msg,&taglist);
bool success = false; bool success = false;
if ( gst_tag_list_get_string( taglist, GST_TAG_TITLE, &string ) && string ) { if ( gst_tag_list_get_string( taglist, GST_TAG_TITLE, &data ) && data ) {
qDebug() << "received tag 'Title': " << QString( string ) ; qDebug() << "received tag 'Title': " << QString( data ) ;
bundle.title = string; bundle.title = data;
success = true; success = true;
} }
if ( gst_tag_list_get_string( taglist, GST_TAG_ARTIST, &string ) && string ) { if ( gst_tag_list_get_string( taglist, GST_TAG_ARTIST, &data ) && data ) {
qDebug() << "received tag 'Artist': " << QString( string ) ; qDebug() << "received tag 'Artist': " << QString( data ) ;
bundle.artist = string; bundle.artist = data;
success = true; success = true;
} }
if ( gst_tag_list_get_string( taglist, GST_TAG_COMMENT, &string ) && string ) { if ( gst_tag_list_get_string( taglist, GST_TAG_COMMENT, &data ) && data ) {
qDebug() << "received tag 'Comment': " << QString( string ) ; qDebug() << "received tag 'Comment': " << QString( data ) ;
bundle.comment = string; bundle.comment = data;
success = true; success = true;
} }
if ( gst_tag_list_get_string( taglist, GST_TAG_ALBUM, &string ) && string ) { if ( gst_tag_list_get_string( taglist, GST_TAG_ALBUM, &data ) && data ) {
qDebug() << "received tag 'Album': " << QString( string ) ; qDebug() << "received tag 'Album': " << QString( data ) ;
bundle.album = string; bundle.album = data;
success = true; success = true;
} }
g_free(string); g_free(data);
gst_tag_list_free(taglist); gst_tag_list_free(taglist);
if (success) if (success)
emit instance->MetadataFound(bundle); emit instance->MetadataFound(bundle);
@ -189,7 +199,7 @@ gboolean GstEnginePipeline::BusCallback(GstBus*, GstMessage* msg, gpointer self)
} }
GstBusSyncReply GstEnginePipeline::BusCallbackSync(GstBus*, GstMessage* msg, gpointer self) { GstBusSyncReply GstEnginePipeline::BusCallbackSync(GstBus*, GstMessage* msg, gpointer self) {
GstEnginePipeline* instance = static_cast<GstEnginePipeline*>(self); GstEnginePipeline* instance = reinterpret_cast<GstEnginePipeline*>(self);
switch (GST_MESSAGE_TYPE(msg)) { switch (GST_MESSAGE_TYPE(msg)) {
case GST_MESSAGE_EOS: case GST_MESSAGE_EOS:
emit instance->EndOfStreamReached(); emit instance->EndOfStreamReached();
@ -204,7 +214,7 @@ GstBusSyncReply GstEnginePipeline::BusCallbackSync(GstBus*, GstMessage* msg, gpo
void GstEnginePipeline::NewPadCallback(GstElement*, GstPad* pad, gboolean, gpointer self) { void GstEnginePipeline::NewPadCallback(GstElement*, GstPad* pad, gboolean, gpointer self) {
GstEnginePipeline* instance = static_cast<GstEnginePipeline*>(self); GstEnginePipeline* instance = reinterpret_cast<GstEnginePipeline*>(self);
GstPad* const audiopad = gst_element_get_pad(instance->audiobin_, "sink"); GstPad* const audiopad = gst_element_get_pad(instance->audiobin_, "sink");
if (GST_PAD_IS_LINKED(audiopad)) { if (GST_PAD_IS_LINKED(audiopad)) {
@ -219,7 +229,7 @@ void GstEnginePipeline::NewPadCallback(GstElement*, GstPad* pad, gboolean, gpoin
void GstEnginePipeline::HandoffCallback(GstPad*, GstBuffer* buf, gpointer self) { void GstEnginePipeline::HandoffCallback(GstPad*, GstBuffer* buf, gpointer self) {
GstEnginePipeline* instance = static_cast<GstEnginePipeline*>(self); GstEnginePipeline* instance = reinterpret_cast<GstEnginePipeline*>(self);
if (instance->forwards_buffers_) { if (instance->forwards_buffers_) {
gst_buffer_ref(buf); gst_buffer_ref(buf);
@ -228,9 +238,9 @@ void GstEnginePipeline::HandoffCallback(GstPad*, GstBuffer* buf, gpointer self)
} }
void GstEnginePipeline::EventCallback(GstPad*, GstEvent* event, gpointer self) { void GstEnginePipeline::EventCallback(GstPad*, GstEvent* event, gpointer self) {
GstEnginePipeline* instance = static_cast<GstEnginePipeline*>(self); GstEnginePipeline* instance = reinterpret_cast<GstEnginePipeline*>(self);
switch(static_cast<int>(event->type)) { switch(event->type) {
case GST_EVENT_EOS: case GST_EVENT_EOS:
emit instance->EndOfStreamReached(); emit instance->EndOfStreamReached();
break; break;
@ -260,7 +270,7 @@ qint64 GstEnginePipeline::length() const {
GstState GstEnginePipeline::state() const { GstState GstEnginePipeline::state() const {
GstState s, sp; GstState s, sp;
if (gst_element_get_state(pipeline_, &s, &sp, kGstStateTimeout) == if (gst_element_get_state(pipeline_, &s, &sp, kGstStateTimeoutNanosecs) ==
GST_STATE_CHANGE_FAILURE) GST_STATE_CHANGE_FAILURE)
return GST_STATE_NULL; return GST_STATE_NULL;

View File

@ -79,7 +79,7 @@ class GstEnginePipeline : public QObject {
void UpdateVolume(); void UpdateVolume();
private: private:
static const int kGstStateTimeout = 10000000; static const int kGstStateTimeoutNanosecs = 10000000;
bool valid_; bool valid_;
QString sink_; QString sink_;