1177 replies to this topic

[christophecvr]

@littlesat ,

 

Guess You mean that the resulting code lines :

		/* required if previous subtitle was external or new one is back to external */
		if (m_change_aktif_subtitle == 1 || m_change_aktif_subtitle == 2)
		{
			if (m_change_aktif_subtitle == 1)
			{
				eDebug("[eServiceMP3] Switch back/restart external subtittle");
				pts_t ppos = 0;
				if (getPlayPosition(ppos) >= 0)
				{
					// seek flush
					seekTo(ppos);
				}
				m_change_aktif_subtitle = 0;
			}
			else if (m_change_aktif_subtitle == 2)
			{
				eDebug("[eServiceMP3] switch to internal subtitle comming from external subtitle.");
				m_change_aktif_subtitle = 3;
			}
			return;
		}

should become :

		if (m_change_aktif_subtitle == 1)
		{
			eDebug("[eServiceMP3] Switch back/restart external subtittle");
			pts_t ppos = 0;
			if (getPlayPosition(ppos) >= 0)
			{
				// seek flush
				seekTo(ppos);
			}
			m_change_aktif_subtitle = 0;
		}
		else if (m_change_aktif_subtitle == 2)
		{
			eDebug("[eServiceMP3] switch to internal subtitle comming from external subtitle.");
			m_change_aktif_subtitle = 3;
		}
		return;

That will not work since we always will have a return and never any subtitle.

 

 

Or what do You mean ?

[littlesat]

Good point...

 

But then put the return inside the if and else if.... (so we have two returns...)

 

But I agree this is subject for discussion. Different coders could have a different meaning here... ...

 

I also like to have descriptions for the 0,1,2,3 conditions. This makes the code more readable -or- event self explainable as you need to describe these conditions in your code (e.g. with a enum thing).

Sorry for bothering you.... as we just try to 'tune' the code the best as we can get...

Edited by littlesat, 7 June 2016 - 15:59.

[christophecvr]

@littlesat ,

 

Yes indeed initiatlly the code was build step by step. And indeed that is why you find the first if(blablab || blabla) . An yes I thaught about to remove that line but :

First like I said the return must be relocated an twice one in the if and a second time in the if else.

 

Second the code is supposed to be for next-master only which is not a beta but experimental I would like to say alpha phase .. then it would not take to long before we move on to pli5

 

However based on more testers when it's pushed to next-master will give a better (wider) important testing base which I do find very important for such things.

Based on the result off testing quit possible that extra changes are needed.

 

After that all tests are done and it is find ok, Then I find it indeed very useful to add a enumeration in the servicemp3.h file for the parameter I added :

int m_change_aktif_subtitle;

This will make it a lot easier to read the code by others and certainly in time when changes must be done.

[betacentauri]

aktif? What is it? Dutch? I guess m_change_current_subtitle is better.

[christophecvr]

aktif? What is it? Dutch? I guess m_change_current_subtitle is better.

I do agree that actually aktif is not dutch neighter english . and yes this could and should be improved. But before it is more important that it is all tested by much more persons then me and much more stb's then my two I have one duo2 and one dm8000.

 

next-master is not published so it would be good the push it there so perhaps we have a much bigger testing base with the own self builders. This is really required for such changes.

 

p.s. I really await results of users and would not be surpised to have some unexpected bad ones. Just cause actually I'm so what shure that the way of working wit the

m_subtitle_sync_timer->stop();
m_subtitle_sync_timer->start(1,true) /*or*/ (ext_timer, true)

//is wrong on it and not required since with gst-1.0 the sub is in sink whit playbin, whic could not be done in gst-0.10.

The point is that we inherit stuff made for the old buggy complete deprecated 0.10 gst code. And this timer is a hack cause gst-0.10 code was so buggy.

[Panther-6]

When can I test OpenPli-5 on my VU+ Solo SE V2? You said, it will be before summer this year. The summer begins already in 2 weeks...

[WanWizard]

We're still aiming for that.

 

Summer ends in 15 weeks, so I'd say before then.

[Erik Slagter]

cvr: newline is also whitespace and it also gets removed if you choose "reformat".

[christophecvr]

Well a can't find any reformat for a c++ std::string .

 

code is

#if GST_VERSION_MAJOR < 1
				std::string line((const char*)GST_BUFFER_DATA(buffer), len);
#else
				std::string line((const char*)map.data, len);
#endif
				/* some media muxers do add an extra new line at the end off a muxed/reencoded srt to ssa codec
					yes that's a bug but looks like a lot off encoders do it now (ffmpeg bug ?) */
				if (!line.empty() && line[line.length()-1] == '\n')
					line.erase(line.length()-1);
				eLog(4, "[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);
				m_subtitle_pages.insert(subtitle_pages_map_pair_t(end_ms, subtitle_page_t(start_ms, end_ms, line)));

the std::line come from the buffer (and it's a bug from handbrake but there are more progam who makes this error) who put and 200A  extra to the end off a subtitle. I wel filed an issue report off that to handbrake that's shure. But this bug is there already since long and not the (best way generaly said is to correct the string by stripping off the \n that must be done other wise whe do have a blank line. in cpp there are more ways but the best is by using erase last char if it's a \n <0x0A>  .

[Panther-6]

We're still aiming for that.

 

Summer ends in 15 weeks, so I'd say before then.

 

I can not wait so long. Please make OpenPli-5 ready in the next days, max. one week. Thank you

[hemertje]

You have too
Remember it's a hobby no work

[WanWizard]

I can not wait so long. Please make OpenPli-5 ready in the next days, max. one week. Thank you

 

I can be clear about that: that is not going to happen. There are external depencies that have to be worked on first, my most optimistic guess whould be "sometime July".

[malakudi]

We could skip OpenPLi-5 and release OpenPLI 16 instead! Indicating the year of the release, like Ubuntu etc.

[vasilich07]

We could skip OpenPLi-5 and release OpenPLI 16 instead! Indicating the year of the release, like Ubuntu etc.

Create at least some new release  

[Erik Slagter]

@cvr: the reformatting takes place at another level, it's not in that part of the code. Just try it.

[christophecvr]

@cvr: the reformatting takes place at another level, it's not in that part of the code. Just try it.

Where and how reformat just does not exist in c and c++

 

basically for a std:string in c++ spaces and new lines are valid string chars where they are in string does not mather.

 

Point is here that for a ssa codec unfortunately the '\N' char is an allowed char but it is not in c or c ++. So our subsink just makes from the char into the ssa codec an 200A (don't ask me why). If I patch the handbrake to use \n instead off \N the media runs perfect by us. Point is that that codec may have \N.

 

So the original codec line into ssa is then

Dialogue: Marked=0,0:00:17.62,0:00:22.76,Default,,0000,0000,0000,,LIGUSTERLAAN
Dialogue: Marked=0,0:01:18.39,0:01:23.22,Default,,0000,0000,0000,,Ik had kunnen weten dat u hier\nwas, professor Anderling .

Will give : out (I adapted for the test the debug output in servicemp3)

<119483.600> [eServiceMP3] ** investigate string ** [BEGIN]LIGUSTERLAAN[END]
HEX OUT : 4C494755535445524C41414E00

<119544.379> [eServiceMP3] ** investigate string ** [BEGIN]Ik had kunnen weten dat u hier 
was, professor Anderling .[END]
HEX OUT : 496B20686164206B756E6E656E20776574656E2064617420752068696572200A7761732C2070726F666573736F7220416E6465726C696E67202E00

That is perfect.

 

But handbrake uses \N now (and this is allowed by that codec)

 

codec :

Dialogue: Marked=0,0:00:17.62,0:00:22.76,Default,,0000,0000,0000,,LIGUSTERLAAN\NDialogue: Marked=0,0:01:18.39,0:01:23.22,Default,,0000,0000,0000,,Ik had kunnen weten dat u hier\Nwas, professor Anderling .\N

the pulled puffer by us is then :

<119483.600> [eServiceMP3] ** investigate string ** [BEGIN]LIGUSTERLAAN 
[END]
HEX OUT : 4C494755535445524C41414E200A00
<119544.379> [eServiceMP3] ** investigate string ** [BEGIN]Ik had kunnen weten dat u hier 
was, professor Anderling . 
[END]
HEX OUT : 496B20686164206B756E6E656E20776574656E2064617420752068696572200A7761732C2070726F666573736F7220416E6465726C696E67202E200A00

So in despite that the codec does use an allowed \N it does not work by us.

[golfdiesel]

We're still aiming for that.

Summer ends in 15 weeks, so I'd say before then.

I can not wait so long. Please make OpenPli-5 ready in the next days, max. one week. Thank you

you know that the guys behind the screens are doing this as a hobby? It is ready when it is ready and when it is ready it will be released.

[christophecvr]

Here the ssa codec patch for ssa codec type made with \N as line retrun instead off \n.

 

This patch is for e2 master.

 

 

Attached Files

•  e2-subtitle-ssa-codec-solution.patch   1.66KB   11 downloads

[littlesat]

I'm doubting if we shoild domthat as this is in fact a bug in the source... This should be solved at handbrake. Also vlc should habe issues with that extra space!

[Erik Slagter]

cvr, you are completely missing the point. The reformatting of the text is done inside the text renderer of enigma. It has nothing to do with either C or C++ or Python. It's a flag you can enable or disable while requesting the text to be rendered.