311 replies to this topic

[athoik]

Patch that make use of UNUSED service reference field by checking m_ref.getData(7) == X is good.

 

We can use wiki for documenting how service reference works. So that normal users will have access to it.

 

#        REFTYPE:FLAGS:STYPE:SID:TSID:ONID:NS:PARENT_SID:PARENT_TSID:UNUSED**
#        D D X X X X X X X X
 
** UNUSED variable is used as buffer flag in servicemp3, value 1 is memory buffer, 2 is file buffer

[littlesat]

And the folders issue?

[pieterg]

Patch that make use of UNUSED service reference field by checking m_ref.getData(7) == X is good.

We can use wiki for documenting how service reference works. So that normal users will have access to it.

#		REFTYPE:FLAGS:STYPE:SID:TSID:ONID:NS:PARENT_SID:PARENT_TSID:UNUSED**
#		D D X X X X X X X X
 
** UNUSED variable is used as buffer flag in servicemp3, value 1 is memory buffer, 2 is file buffer

But document in code as well, code'll exist long after the wiki has gone.

[athoik]

And the folders issue?

Folders issue exist if we are setting flags field to an odd value. Since we are going to use UNUSED field we gonna be ok.

I will add also comments in the code about UNUSED field usage.

[athoik]

Usefull comments inserted
 
Hopefully now everything is ok!

--- a/lib/service/servicemp3.cpp
+++ b/lib/service/servicemp3.cpp
@@ -450,11 +450,20 @@ eServiceMP3::eServiceMP3(eServiceReference ref)
         if (m_useragent.empty())
             m_useragent = "Enigma2 Mediaplayer";
         m_extra_headers = eConfigManager::getConfigValue("config.mediaplayer.extraHeaders");
-        if (strstr(filename, " buffer=1"))
+
+        /*
+         * UNUSED variable from service reference is now used as buffer flag for gstreamer
+         * REFTYPE:FLAGS:STYPE:SID:TSID:ONID:NS:PARENT_SID:PARENT_TSID:UNUSED
+         *   D  D X X X X X X X X
+         * 4097:0:1:0:0:0:0:0:0:0:URL:NAME (no buffering)
+         * 4097:0:1:0:0:0:0:0:0:1:URL:NAME (is memory buffering)
+         * 4097:0:1:0:0:0:0:0:0:2:URL:NAME (is disk buffering)
+         */
+        if ( m_ref.getData(7) == 1 )
         {
             m_use_prefillbuffer = true;
         }
-        else if (strstr(filename, " buffer=2"))
+        else if ( m_ref.getData(7) == 2 )
         {
             /* progressive download buffering */
             if (::access("/hdd/movie", X_OK) >= 0)

Attached Files

•  0001-UNUSED-variable-from-service-reference-is-now-used-a.patch.txt   1.52KB   5 downloads

[pieterg]

I mean the == 1 and == 2, that should be something like (flags | BUFFERING_ENABLED) and (flags | PROGRESSIVE_DOWNLOAD_BUFFERING_ENABLED), so it documents itself.
But the comments, in addition, are good as well of course. (but they should be moved to where the defines are, I think in the header file)

[athoik]

@pieterg don't get mad with me, but i am not used, writing code with binary comparisons. So please be patient.

I am thinking the following:

1. edit servicemp3.h and add defines after includes

/*
 * UNUSED variable from service reference is now used as buffer flag for gstreamer
 * REFTYPE:FLAGS:STYPE:SID:TSID:ONID:NS:PARENT_SID:PARENT_TSID:UNUSED
 *   D  D X X X X X X X X
 * 4097:0:1:0:0:0:0:0:0:0:URL:NAME (NO BUFFERING)
 * 4097:0:1:0:0:0:0:0:0:1:URL:NAME (BUFFERING_ENABLED)
 * 4097:0:1:0:0:0:0:0:0:2:URL:NAME (PROGRESSIVE_DOWNLOAD_BUFFERING_ENABLED)
 */
#define BUFFERING_ENABLED 1
#define PROGRESSIVE_DOWNLOAD_BUFFERING_ENABLED 2

 
2. change servicemp3.cpp check if buffering is enabled

        if ( m_ref.getData(7) & BUFFERING_ENABLED )
        else if ( m_ref.getData(7) & PROGRESSIVE_DOWNLOAD_BUFFERING_ENABLED )

Or like this?

        if ( m_ref.getData(7) == BUFFERING_ENABLED )
        else if ( m_ref.getData(7) == PROGRESSIVE_DOWNLOAD_BUFFERING_ENABLED )

You are mentioning check like this flags | BUFFER_ENABLED, but i am not sure how i can do that.

Thanks

[pieterg]

sorry, my mistake. I meant 'm_ref.getData(7) & BUFFERING_ENABLED'.

I think these flags should be bitfields, they can be combined.
(just in case we need some other features in the future, they have to be combined with the buffering flags)
So I think we only need to check for the progressive download bit, when the buffering bit is nonzero.
At least, if I remember correctly, we need to enable gstreamer buffering, if we want progressive download buffering.

[littlesat]

Good idea to make them combined...

[athoik]

Hello,

Maybe it's not ideal, but i hope this one it's good enough to pushed.

--- a/lib/service/servicemp3.cpp
+++ b/lib/service/servicemp3.cpp
@@ -21,6 +21,20 @@

 #define HTTP_TIMEOUT 30

+/*
+ * UNUSED variable from service reference is now used as buffer flag for gstreamer
+ * REFTYPE:FLAGS:STYPE:SID:TSID:ONID:NS:PARENT_SID:PARENT_TSID:UNUSED
+ *   D  D X X X X X X X X
+ * 4097:0:1:0:0:0:0:0:0:0:URL:NAME (NO BUFFERING)
+ * 4097:0:1:0:0:0:0:0:0:1:URL:NAME (BUFFERING_ENABLED)
+ * 4097:0:1:0:0:0:0:0:0:2:URL:NAME (PROGRESSIVE_DOWNLOAD_BUFFERING_ENABLED)
+ */
+typedef enum
+{
+       BUFFERING_ENABLED                       = 0x00000001,
+       PROGRESSIVE_DOWNLOAD_BUFFERING_ENABLED  = 0x00000002
+} eServiceMP3Flags;
+
 typedef enum
 {
        GST_PLAY_FLAG_VIDEO         = 0x00000001,
@@ -450,11 +464,11 @@ eServiceMP3::eServiceMP3(eServiceReference ref)
                if (m_useragent.empty())
                        m_useragent = "Enigma2 Mediaplayer";
                m_extra_headers = eConfigManager::getConfigValue("config.mediaplayer.extraHeaders");
-               if (strstr(filename, " buffer=1"))
+               if ( m_ref.getData(7) & BUFFERING_ENABLED )
                {
                        m_use_prefillbuffer = true;
                }
-               else if (strstr(filename, " buffer=2"))
+               else if ( m_ref.getData(7) & PROGRESSIVE_DOWNLOAD_BUFFERING_ENABLED )
                {
                        /* progressive download buffering */
                        if (::access("/hdd/movie", X_OK) >= 0)

Attached Files

•  0001-UNUSED-variable-from-service-reference-is-now-used-a.patch.txt   1.82KB   3 downloads

Edited by athoik, 22 November 2013 - 18:56.

[pieterg]

The 'else' now causes the progressive download buffering flag to be ignored, when the buffering flag is set.
I think this is a bit counter-intuitive.

[athoik]

But m_use_prefillbuffer is enabled also on progressive download. Or i miss something?

if ( m_ref.getData(7) & BUFFERING_ENABLED )
{
   m_use_prefillbuffer = true;
}
else if ( m_ref.getData(7) & PROGRESSIVE_DOWNLOAD_BUFFERING_ENABLED )
{
   /* progressive download buffering */
   if (::access("/hdd/movie", X_OK) >= 0)
   {
      /* It looks like /hdd points to a valid mount, so we can store a download buffer on it */
      m_download_buffer_path = "/hdd/gstreamer_XXXXXXXXXX";
   }
   m_use_prefillbuffer = true;
}

[littlesat]

You do not miss something...... But the else can still be removed...

[pieterg]

But m_use_prefillbuffer is enabled also on progressive download.

Yes, therefore it makes sense to allow both buffer flags to be set, at the same time.

[athoik]

>>> if 3 & 1:
...    print True
...
True
>>> if 3 & 2:
...    print True
...
True

0x3? (It will never go to else?)

[malakudi]

I guess this is what Pieterg prefers:

So you would either set 1 for memory buffering, 2 or 3 disk buffering. If you remove m_use_prefillbuffer=true on the second block, then you would have to set 1 for memory buffering, 3 for disk buffering.

 

if ( m_ref.getData(7) & BUFFERING_ENABLED )
{
   m_use_prefillbuffer = true;
}
if ( m_ref.getData(7) & PROGRESSIVE_DOWNLOAD_BUFFERING_ENABLED )
{
   /* progressive download buffering */
   if (::access("/hdd/movie", X_OK) >= 0)
   {
      /* It looks like /hdd points to a valid mount, so we can store a download buffer on it */
      m_download_buffer_path = "/hdd/gstreamer_XXXXXXXXXX";
   }
   m_use_prefillbuffer = true;
}

[athoik]

Yes, just removing else from patch must be ok.

(Python interpreter really helps to quick test binary)

I was also thinking instead call ref.getData(x) to use ref.onid,ref.sid, it's more clear. What do you think?

[pieterg]

Yes, just removing else from patch must be ok.

or even put the progressive download buffering within the buffer check.
The point is, I think we should use the buffer flags as bitfields, not actual values. (so we can combine them with future settings).

I was also thinking instead call ref.getData(x) to use ref.onid,ref.sid, it's more clear. What do you think?

where do you mean? servicemp3 does not use onid and sid as far as I remember?

[athoik]

Eg. Instead calling ref.getData(7) to call ref.unused witch is closer to service reference layout.

[pieterg]

Please explain in more detail?