97 replies to this topic

[malakudi]

I have to say in favor of christophecvr that we are making a way big deal for a simple two line comment patch. We should skip this "bureaucracy" for such a simple patch. 

[littlesat]

I think we should not go for commented eDebug lines and quickly apply a quick fix for something that is in there for years.... I think it is better to invest our energy to really solve this by adding debug levels.... So you can enable/dispable it e.g. with a parameter when enigma2 is executed... And put e.g. servicemp3.cpp eDebugs to a level that is off by default...
I thinkk a quick apply makes not many sense since we already commented out the most importee showstopper.... Or do we really have a big argument with the other ones?

[mirakels]

Apart from debug levels the issue in this thread can be discussed on its own. Debug levels are a separate 'project'.

 

PS: I promoted myself from KING to president of the Universe so hold your breath or you will be assimilated.

 

I applied the most important extreme priority bug a while ago now with some comments in this thread on how other issues can be applied too, but all I see is further yelling and demanding and bad attitude. So if there is no more constructive information nothing will be done until someone else in some future times runs into this and fixes it. To bad, as other people already said: this is hobby no work ( exept that I seem to be the master now and can dictate everybody; well I told you what to do so do so).

And a tip for chris: 'right'  your comments in English AND Dutch. Maybe that helps to understand what you mean. Am I 'write' or wrong?

[theparasol]

I vote +1 for debuglevels too... Its not that hard to implement and will make logging a lot cleaner / readable and as a bonus we will avoid running into issues like Chris discovered.

Anyway, I updated my XP1000 and now my radiostreams are playing quick like 2 years ago befor the stream bufferingpatch of Athoik.

No more 10 to 15 seconds waiting till stream starts    (yes, XP1000 has very slow cpu hence the long timeout, it needed to parse xxxxxx useless locked debug print messages)

[athoik]

Anyway, I updated my XP1000 and now my radiostreams are playing quick like 2 years ago befor the stream bufferingpatch of Athoik.

What patch exacly?

[theparasol]

I'm not too sure but it must be around these commits or perhaps a bit later but not later than februari 2014

 

https://github.com/O...47d21a3ac1532b5

 

or

 

https://github.com/O...47d21a3ac1532b5

 

Anyway, way back. And my memory fails on me. I only remember trying to listen to a shoutcast stream and all of sudden spinners were there.

Its a pity I didn't listen every day back then, it would be more easy to find the exact commit.

 

I apology for pointing at you in person about something in the past. I shouldn't have done that in the first place especially since I'm not 100% sure what commit exactly introduced it.

Lets forget about it, I'm glad we now know what is causing the spinners.

Edited by theparasol, 19 January 2016 - 01:23.

[littlesat]

I do not think so.... Here the code will only go for a different road when buffer preload is indicated by the serviceref...

[malakudi]

I think we should not go for commented eDebug lines and quickly apply a quick fix for something that is in there for years.... I think it is better to invest our energy to really solve this by adding debug levels.... So you can enable/dispable it e.g. with a parameter when enigma2 is executed... And put e.g. servicemp3.cpp eDebugs to a level that is off by default...
I thinkk a quick apply makes not many sense since we already commented out the most importee showstopper.... Or do we really have a big argument with the other ones?

 

After trying hard to follow all chrisstophe's posts, it seems the other two are also important (one for subtitles and the other I don't remember).

Of course the correct way of fixing this is to implement a whole new debug/trace system with levels etc. But applying a two line comment patch will not affect the maturity level of the code. Anyone with commit access, just apply it, is not a big deal.

Edited by malakudi, 19 January 2016 - 08:38.

[MastaG]

I'm applying the rest of them in my next release:

e.g.

From a2c49a351731983dbdec0bce84d3907107ba7c16 Mon Sep 17 00:00:00 2001
From: christophecvr <stefansat@telenet.be>
Date: Wed, 13 Jan 2016 12:55:04 +0100
Subject: [PATCH] Commented a couple off eDebug lines as they cause a critical
 stb freeze.

 Some eDebug lines do issue to much information to the log.
 They will cause to cpu intensif standard working for nothing.
 The result is sand keeper, temporary stb freeze up to full stb freeze or crash.
 Some media can't be played due to that. The logs if required will also be almost unreadable.
 I now only started by commenting some lines into servicemp3.cpp.
 Why commenting and not removing. In order to search a possible error,
 the developer can just re enable for his own e2 trouble shooting version.
 When he find the error just recomment the log output.

    modified:   lib/service/servicemp3.cpp
---
 lib/service/servicemp3.cpp | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/lib/service/servicemp3.cpp b/lib/service/servicemp3.cpp
index 5af48e8..c7f0ca8 100644
--- a/lib/service/servicemp3.cpp
+++ b/lib/service/servicemp3.cpp
@@ -2203,8 +2203,8 @@ void eServiceMP3::HandleTocEntry(GstMessage *msg)
             GstTocEntry *entry = static_cast<GstTocEntry*>(i->data);
             if (gst_toc_entry_get_entry_type (entry) == GST_TOC_ENTRY_TYPE_EDITION)
             {
-                /* extra debug info for testing purposes CVR should_be_removed later on */
-                eDebug("[eServiceMP3] toc_type %s", gst_toc_entry_type_get_nick(gst_toc_entry_get_entry_type (entry)));
+                /* extra debug info for testing purposes should_be_removed later on */
+                //eDebug("[eServiceMP3] toc_type %s", gst_toc_entry_type_get_nick(gst_toc_entry_get_entry_type (entry)));
                 gint y = 0;
                 for (GList* x = gst_toc_entry_get_sub_entries (entry); x; x = x->next)
                 {
@@ -2234,9 +2234,9 @@ void eServiceMP3::HandleTocEntry(GstMessage *msg)
                                 m_cue_entries.insert(cueEntry(pts, type));
                                 m_cuesheet_changed = 1;
                                 m_event((iPlayableService*)this, evCuesheetChanged);
-                                /* extra debug info for testing purposes CVR should_be_removed later on */
-                                eDebug("[eServiceMP3] toc_subtype %s,Nr = %d, start= %#"G_GINT64_MODIFIER "x",
-                                        gst_toc_entry_type_get_nick(gst_toc_entry_get_entry_type (sub_entry)), y + 1, pts);
+                                /* extra debug info for testing purposes should_be_removed later on */
+                                /*eDebug("[eServiceMP3] toc_subtype %s,Nr = %d, start= %#"G_GINT64_MODIFIER "x",
+                                        gst_toc_entry_type_get_nick(gst_toc_entry_get_entry_type (sub_entry)), y + 1, pts);*/
                             }
                         }
                         y++;
@@ -2248,7 +2248,8 @@ void eServiceMP3::HandleTocEntry(GstMessage *msg)
     }
     else
     {
-        eDebug("[eServiceMP3] TOC entry from source %s not used", GST_MESSAGE_SRC_NAME(msg));
+        //eDebug("[eServiceMP3] TOC entry from source %s not used", GST_MESSAGE_SRC_NAME(msg));
+        ;
     }
 }
 #endif
@@ -2522,11 +2523,11 @@ void eServiceMP3::pullSubtitle(GstBuffer *buffer)
         }
         gint64 buf_pos = GST_BUFFER_PTS(buffer);
         size_t len = map.size;
-        eDebug("[eServiceMP3] gst_buffer_get_size %zu map.size %zu", gst_buffer_get_size(buffer), len);
+        //eDebug("[eServiceMP3] gst_buffer_get_size %zu map.size %zu", gst_buffer_get_size(buffer), len);
 #endif
         gint64 duration_ns = GST_BUFFER_DURATION(buffer);
         int subType = m_subtitleStreams[m_currentSubtitleStream].type;
-        eDebug("[eServiceMP3] pullSubtitle type=%d size=%zu", subType, len);
+        //eDebug("[eServiceMP3] pullSubtitle type=%d size=%zu", subType, len);
         if ( subType )
         {
             if ( subType < stVOB )
@@ -2543,7 +2544,7 @@ void eServiceMP3::pullSubtitle(GstBuffer *buffer)
 #else
                 std::string line((const char*)map.data, len);
 #endif
-                eDebug("[eServiceMP3] got new text subtitle @ buf_pos = %lld ns (in pts=%lld), dur=%lld: '%s' ", buf_pos, buf_pos/11111, duration_ns, line.c_str());
+                //eDebug("[eServiceMP3] got new text subtitle @ buf_pos = %lld ns (in pts=%lld), dur=%lld: '%s' ", buf_pos, buf_pos/11111, duration_ns, line.c_str());
 
                 uint32_t start_ms = ((buf_pos / 1000000ULL) * convert_fps) + (delay / 90);
                 uint32_t end_ms = start_ms + (duration_ns / 1000000ULL);
@@ -2784,7 +2785,7 @@ PyObject *eServiceMP3::getCutList()
 
     return list;
 }
-/* cuesheet CVR */
+/* cuesheet */
 void eServiceMP3::setCutList(ePyObject list)
 {
     if (!PyList_Check(list))
@@ -2816,7 +2817,7 @@ void eServiceMP3::setCutList(ePyObject list)
         pts_t pts = PyLong_AsLongLong(ppts);
         int type = PyInt_AsLong(ptype);
         m_cue_entries.insert(cueEntry(pts, type));
-        eDebug("[eServiceMP3] adding %08llx, %d", pts, type);
+        //eDebug("[eServiceMP3] adding %08llx, %d", pts, type);
     }
     m_cuesheet_changed = 1;
     m_event((iPlayableService*)this, evCuesheetChanged);
@@ -2915,7 +2916,7 @@ void eServiceMP3::loadCuesheet()
     }
     else
     {
-        eDebug("[eServiceMP3] skip loading cuesheet multiple times");
+        //eDebug("[eServiceMP3] skip loading cuesheet multiple times");
         return;
     }
 
--
1.9.1

 

This fixes the spinner you sometimes get when enigma2 is processing subtitles.

Verified on DM800Se, DM800, spark7162 and zgemma star h2h.

 

So I vote the complete patch in the trunk as well :-)

[littlesat]

So you go for the work-a-round instead of the real solution...???

 

I would at least recommend to start with eDebug levels...

 

On my boxes I use daily (F1, ET10K, Sole4K) I do not experience spinners with processing subtitles...????

 

So I think we better could do something like this...

 

eDebug("[eServiceMP3] gst_buffer_get_size %zu map.size %zu", gst_buffer_get_size(buffer), len);
eDebug("128,[eServiceMP3] gst_buffer_get_size %zu map.size %zu", gst_buffer_get_size(buffer), len);

 

and add a function to debug that for now somehow dumps the log.... and later you can set a debug level...

 

I would recommend to go at least to prepare level codes.... (no integer in front = always show)

Edited by littlesat, 21 January 2016 - 09:36.

[mirakels]

as said before debug levels is on its way but is a different thing fron useless edebugs.
messages like bufsize=%d seem actual development time debugs that should not be in the 'production' code at all.
So when we have debuglevels (which we will have shortly) does not mean we can just flood enigma with debug lines reasoning they will not be shown anyway when setting the correct debug level.

So if there are such now meaningless edebugs they should be removed by issueing pull request with a clear explanation why they were there and why they can be removed.

hope this is ckear by now (finally).

[littlesat]

So you mean simply fully removing them instead of commenting them out....

[mirakels]

yes if they were used to establish understanding a specific issue that is now resolved, so the messages are not needed anymore, they can simply be removed.
but if it is foreseen that they might be usefull in the future they can be commented out.

in future, using debuglevels, this may become easier. And with a tagging system i'm thinking about it may be even more flexible...

[MiLo]

#include <stdio.h>
static const int debug = 0;

void blah(void)
{
  if (debug)
    printf("Hello");
  printf("World");
}

If you compile this (with optimization enabled), the compiler is smart enough to remove the unreachable code path. You'll not find the string "Hello" in the output, not even in the .o file.

If you have useful comments that should be enabled with simple switches, just do it like that. This will check your debug code for errors, but will not output any actual code until you explicitly enable it. Using #defines for this purpose makes the compiler ignore it completely, so it will bitrot an eventually cannot be compiled any longer if you trigger the #define.

This also works if you define levels. If at some point you set:
 

static const int debug_level = 4;

#define eLog(level, ...) if ((level) > debug_level) printf(...)

This will make the compiler remove all code and data below the set debug level, but still make your live miserable if you made a syntax error or so in your log statement. Very convenient if you want to do some complex logging, just put it into a simple block and you're done.

This of course requires that there is no code path that sets "debug_level" to a different value (hence the "const").

Edited by MiLo, 23 January 2016 - 20:47.

[theparasol]

That will work just fine at compile time but the image users download and install on their boxes will have no debug level at all I presume.

Isn't it an idea to use runtime debuglevels and some kind of commandline switch like enigma2 -d4 to log some additional debugging loglines?

That way if someone reports an error in the forum its possible to tell:

 

Can you make me a -d4 log?

kill 4
enigma2 -d4 > /tmp/d4log.txt

Edited by theparasol, 23 January 2016 - 21:15.

[MiLo]

If you do that, you might as well compile and send them a debug enigma version.

[christophecvr]

Hello to all,

 

I'm happy to see that a couple off moderators and core members off this site do see the major problem about the debugging currently used into enigma2.

 

Yes  it's time to do something at it . Do we need to rush and put all up site down at once  ?? now , it would just make things worse.

 

But in a very first stage:

Just eliminate the worst  off them.

I just provided One single patch for a couple off them in servicemp3.cpp . That's all . And yes , I used commenting, this with the goal off constructive positive conversation and discussion say either brainstorm about the whole debugging into enigma2 .

 

So In my mind quit simple steps.

1) eliminate the real bugs (and they do at this time) in a first place. This by just commenting them. (should have been done yesterday)

2) For that I did wanted to launch a positive constructive dialogue   with all.

 

Ok I'm a bad writer , but sorry : I reviewed the whole topic , From the very few lines what I just told was said .

This all very honest without any lie and or fantasy . Unfortunately some Core members really do not understand the impact from the wrong use off eDebug.

 

The problem is that eDebug is used at all times also for the regular user. In normal use there should be not any debugging actif, ... but somehow we well need a crash report (and the code is such that we can't compile without debug active).

This is now since long in enigma2 and will take a lot off new implementations say rework off code on a tremendous extend.

 

That is the only reason that I first push for point 1 Do comment the non needed eDebug for now the few couples off real problem makers in servicemp3.cpp I did , but there are other modules with similar to much eDebug lines active on a permanent base.

 

Then we have time (and that will take a lot off time to develop a serious debugging system into e2  like for example gstreamer does.) I'm not an expert in such things, and it's clear that a lot off core members are not also.

[Erik Slagter]

#defines are evil anyway. Just like MiLo says, they're not required for many conditional compilation constructions. #defines are for people that lack proper C/C++ knowledge.