Mailing List Archive

regression with 7f00642b
Guys,

https://github.com/MythTV/mythtv/commit/7f00642ba11eb0d9d633a23ce74e5b695c05153e
causes for me failed recordings/livetv from multiple DVB-S/S2 SAT mplexes with:

DTVRec[17]: Music Choice program detected

it looks this commit is clear regression


_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: regression with 7f00642b [ In reply to ]
On Thu, May 10, 2018 at 4:20 PM, Piotr Oniszczuk
<piotr.oniszczuk@gmail.com> wrote:

> it looks this commit is clear regression

I was afraid there was a side effect of not calling
those other routines (and mentioned to David
that someone with more expertice regarding
the mpeg table manipulation might need to
review those). Sorry.
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: regression with 7f00642b [ In reply to ]
On Thu, 2018-05-10 at 16:32 +0000, Gary Buhrmaster wrote:
> On Thu, May 10, 2018 at 4:20 PM, Piotr Oniszczuk
> <piotr.oniszczuk@gmail.com> wrote:
>
> > it looks this commit is clear regression
>
> I was afraid there was a side effect of not calling
> those other routines (and mentioned to David
> that someone with more expertice regarding
> the mpeg table manipulation might need to
> review those). Sorry.

I looked for side effects, and must have missed it. Looking again, I
would say it must be the call to AppendStream. Sorry. I'll revert it
later this afternoon.

David

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: regression with 7f00642b [ In reply to ]
On 05/10/2018 12:20 PM, Piotr Oniszczuk wrote:
> Guys,
>
> https://github.com/MythTV/mythtv/commit/7f00642ba11eb0d9d633a23ce74e5b695c05153e
> causes for me failed recordings/livetv from multiple DVB-S/S2 SAT mplexes with:
>
> DTVRec[17]: Music Choice program detected
>
> it looks this commit is clear regression
>
>
>
If it says "Music choice program detected" that will not normally cause
a failure. It indicates that there is a very slow frame rate. It does
not treat those much differently, just uses increased timeout values,
which should not cause a problem. Probably the "Music choice detected"
is a symptom of something else happening.

Peter
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: regression with 7f00642b [ In reply to ]
Peter,
With this commit, many mplexes started to have following codec information:

May 10 16:44:31 mythtv mythbackend[7670]: TVRec[17]: TuningNewRecorder - CreateRecorder()
May 10 16:44:34 mythtv mythbackend[7670]: DTVRec[17]: Music Choice program detected
May 10 16:44:36 mythtv mythbackend[7670]: TVRec[17]: Changing from WatchingLiveTV to None
May 10 16:44:36 mythtv mythbackend[7670]: Finished Recording: Container: MPEG2-TS Video Codec: none (0x0 A/R: 0 25fps) Audio Codec: ac3
May 10 16:44:36 mythtv mythbackend[7670]: TVRec[17]: FinishedRecording(15457_2018-05-10T14:44:31Z) damaged recq:<RecordingQuality overall_score=„0"

pls look at video codec…

looking on recorder code, I see that ‚Music Choice program detected’ detection is based stream rate evaluating.
There is no video codec reported - I suspect recorded stream has filtered-out video part - so bit rate becomes v.low.
this low bitrate causes that dtvrecorder detects such stream as Music Choice program….



> Wiadomo?? napisana przez Peter Bennett <pb.mythtv@gmail.com> w dniu 10.05.2018, o godz. 19:05:
>
>
>
> On 05/10/2018 12:20 PM, Piotr Oniszczuk wrote:
>> Guys,
>>
>> https://github.com/MythTV/mythtv/commit/7f00642ba11eb0d9d633a23ce74e5b695c05153e
>> causes for me failed recordings/livetv from multiple DVB-S/S2 SAT mplexes with:
>>
>> DTVRec[17]: Music Choice program detected
>>
>> it looks this commit is clear regression
>>
>>
> If it says "Music choice program detected" that will not normally cause a failure. It indicates that there is a very slow frame rate. It does not treat those much differently, just uses increased timeout values, which should not cause a problem. Probably the "Music choice detected" is a symptom of something else happening.
>
> Peter
> _______________________________________________
> mythtv-dev mailing list
> mythtv-dev@mythtv.org
> http://lists.mythtv.org/mailman/listinfo/mythtv-dev
> http://wiki.mythtv.org/Mailing_List_etiquette
> MythTV Forums: https://forum.mythtv.org

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: regression with 7f00642b [ In reply to ]
On Thu, 2018-05-10 at 13:02 -0400, David Hampton wrote:
> On Thu, 2018-05-10 at 16:32 +0000, Gary Buhrmaster wrote:
> > On Thu, May 10, 2018 at 4:20 PM, Piotr Oniszczuk
> > <piotr.oniszczuk@gmail.com> wrote:
> >
> > > it looks this commit is clear regression
> >
> > I was afraid there was a side effect of not calling
> > those other routines (and mentioned to David
> > that someone with more expertice regarding
> > the mpeg table manipulation might need to
> > review those). Sorry.
>
> I looked for side effects, and must have missed it. Looking again, I
> would say it must be the call to AppendStream. Sorry. I'll revert
> it
> later this afternoon.

I have reverted this patch. I don't have the resources to recreate the
original crash, nor the crash that was caused by Gary's patch. I'd be
happy to commit a fix that anyone comes up with, but I'm afraid I can't
help in the creation of said fix. Sorry.

David

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: regression with 7f00642b [ In reply to ]
On Thu, May 10, 2018 at 7:14 PM, David Hampton <mythtv@love2code.net> wrote:

> I have reverted this patch. I don't have the resources to recreate the
> original crash, nor the crash that was caused by Gary's patch. I'd be
> happy to commit a fix that anyone comes up with, but I'm afraid I can't
> help in the creation of said fix. Sorry.

Attempt 2.

This proposed patch is fundamentally ugly, but since passing
in a NULL value to memcpy is undefined (my first thought
was check for empty, and pass in a NULL for the pointer
in the case the size is zero, but a memcpy from NULL,
even with a zero length of the copy, is undefined by standards,
that is not acceptable), this seems like an approach that will
work for the moment and cause the side effects of the rest
of calling the functions to continue working.

It works for my test case. I would like to hear from some
dvb-s/s2 users.




diff --git a/mythtv/libs/libmythtv/mpeg/mpegtables.cpp
b/mythtv/libs/libmythtv/mpeg/mpegtables.cpp
index 3120b821f6..dbe87d5899 100644
--- a/mythtv/libs/libmythtv/mpeg/mpegtables.cpp
+++ b/mythtv/libs/libmythtv/mpeg/mpegtables.cpp
@@ -445,7 +445,12 @@ ProgramMapTable* ProgramMapTable::Create(
uint len = global_desc[i][1] + 2;
gdesc.insert(gdesc.end(), global_desc[i], global_desc[i] + len);
}
- pmt->SetProgramInfo(&gdesc[0], gdesc.size());
+
+ // TODO: Fix this properly?
+ if (gdesc.empty())
+ pmt->SetProgramInfo((unsigned char *)"", 0);
+ else
+ pmt->SetProgramInfo(&gdesc[0], gdesc.size());

for (uint i = 0; i < count; i++)
{
@@ -457,7 +462,11 @@ ProgramMapTable* ProgramMapTable::Create(
prog_desc[i][j], prog_desc[i][j] + len);
}

- pmt->AppendStream(pids[i], types[i], &pdesc[0], pdesc.size());
+ // TODO: Fix this properly?
+ if (pdesc.empty())
+ pmt->AppendStream(pids[i], types[i], (unsigned char *)"", 0);
+ else
+ pmt->AppendStream(pids[i], types[i], &pdesc[0], pdesc.size());
}
pmt->Finalize();
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: regression with 7f00642b [ In reply to ]
reverse that change and pass &gdesc.front() instead

(and &pdesc.front())

On 11 May 2018 at 08:11, Gary Buhrmaster <gary.buhrmaster@gmail.com> wrote:
> On Thu, May 10, 2018 at 7:14 PM, David Hampton <mythtv@love2code.net> wrote:
>
>> I have reverted this patch. I don't have the resources to recreate the
>> original crash, nor the crash that was caused by Gary's patch. I'd be
>> happy to commit a fix that anyone comes up with, but I'm afraid I can't
>> help in the creation of said fix. Sorry.
>
> Attempt 2.
>
> This proposed patch is fundamentally ugly, but since passing
> in a NULL value to memcpy is undefined (my first thought
> was check for empty, and pass in a NULL for the pointer
> in the case the size is zero, but a memcpy from NULL,
> even with a zero length of the copy, is undefined by standards,
> that is not acceptable), this seems like an approach that will
> work for the moment and cause the side effects of the rest
> of calling the functions to continue working.
>
> It works for my test case. I would like to hear from some
> dvb-s/s2 users.
>
>
>
>
> diff --git a/mythtv/libs/libmythtv/mpeg/mpegtables.cpp
> b/mythtv/libs/libmythtv/mpeg/mpegtables.cpp
> index 3120b821f6..dbe87d5899 100644
> --- a/mythtv/libs/libmythtv/mpeg/mpegtables.cpp
> +++ b/mythtv/libs/libmythtv/mpeg/mpegtables.cpp
> @@ -445,7 +445,12 @@ ProgramMapTable* ProgramMapTable::Create(
> uint len = global_desc[i][1] + 2;
> gdesc.insert(gdesc.end(), global_desc[i], global_desc[i] + len);
> }
> - pmt->SetProgramInfo(&gdesc[0], gdesc.size());
> +
> + // TODO: Fix this properly?
> + if (gdesc.empty())
> + pmt->SetProgramInfo((unsigned char *)"", 0);
> + else
> + pmt->SetProgramInfo(&gdesc[0], gdesc.size());
>
> for (uint i = 0; i < count; i++)
> {
> @@ -457,7 +462,11 @@ ProgramMapTable* ProgramMapTable::Create(
> prog_desc[i][j], prog_desc[i][j] + len);
> }
>
> - pmt->AppendStream(pids[i], types[i], &pdesc[0], pdesc.size());
> + // TODO: Fix this properly?
> + if (pdesc.empty())
> + pmt->AppendStream(pids[i], types[i], (unsigned char *)"", 0);
> + else
> + pmt->AppendStream(pids[i], types[i], &pdesc[0], pdesc.size());
> }
> pmt->Finalize();
> _______________________________________________
> mythtv-dev mailing list
> mythtv-dev@mythtv.org
> http://lists.mythtv.org/mailman/listinfo/mythtv-dev
> http://wiki.mythtv.org/Mailing_List_etiquette
> MythTV Forums: https://forum.mythtv.org
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: regression with 7f00642b [ In reply to ]
On 11 May 2018 at 09:53, Jean-Yves Avenard <jyavenard@gmail.com> wrote:
> reverse that change and pass &gdesc.front() instead
>
> (and &pdesc.front())

actually, &*gdesc.begin() may be better.
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: regression with 7f00642b [ In reply to ]
or if c++11 is finally used: gdesc.data()

On 11 May 2018 at 10:04, Jean-Yves Avenard <jyavenard@gmail.com> wrote:
> On 11 May 2018 at 09:53, Jean-Yves Avenard <jyavenard@gmail.com> wrote:
>> reverse that change and pass &gdesc.front() instead
>>
>> (and &pdesc.front())
>
> actually, &*gdesc.begin() may be better.
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: regression with 7f00642b [ In reply to ]
On Fri, 2018-05-11 at 10:11 +0200, Jean-Yves Avenard wrote:
> or if c++11 is finally used: gdesc.data()

All the builds use c++11. :-)

David

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: regression with 7f00642b [ In reply to ]
On Fri, 2018-05-11 at 07:33 -0400, David Hampton wrote:
> On Fri, 2018-05-11 at 10:11 +0200, Jean-Yves Avenard wrote:
> > or if c++11 is finally used: gdesc.data()
>
> All the builds use c++11. :-)

I need to correct that statement. All the builds of master use c++11.
I will need to check the 29 and 0.28 builds to see what flags they
specify.

David

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: regression with 7f00642b [ In reply to ]
On Fri, May 11, 2018 at 8:04 AM, Jean-Yves Avenard <jyavenard@gmail.com> wrote:
> On 11 May 2018 at 09:53, Jean-Yves Avenard <jyavenard@gmail.com> wrote:
>> reverse that change and pass &gdesc.front() instead
>>
>> (and &pdesc.front())
>
> actually, &*gdesc.begin() may be better.

My recollection (of the standard) is that if the
vector is empty, neither begin() nor front() are
defined and/or can be dereferenced. And these
are cases where the vector is empty.

But I would certainly defer to someone who
knows the standards better than I.
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: regression with 7f00642b [ In reply to ]
On Fri, 2018-05-11 at 07:45 -0400, David Hampton wrote:
> On Fri, 2018-05-11 at 07:33 -0400, David Hampton wrote:
> > On Fri, 2018-05-11 at 10:11 +0200, Jean-Yves Avenard wrote:
> > > or if c++11 is finally used: gdesc.data()
> >
> > All the builds use c++11. :-)
>
> I need to correct that statement. All the builds of master use
> c++11.
> I will need to check the 29 and 0.28 builds to see what flags they
> specify.

The settings.pro files for 0.28/29/master all unconditionally set the
c++11 flag.

David

_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: regression with 7f00642b [ In reply to ]
The key is that they aren't nullptr. As such, the behaviour isn't undefined.

As it's called with length set to 0, it is fine.

Vector::data() is the one you would want to use though.

Get Firefox on Android
________________________________
From: Gary Buhrmaster
Sent: Friday, 11 May 2018 08:05
To: Development of MythTV
Subject: Re: [mythtv] regression with 7f00642b

On Fri, May 11, 2018 at 8:04 AM, Jean-Yves Avenard <jyavenard@gmail.com> wrote:
> On 11 May 2018 at 09:53, Jean-Yves Avenard <jyavenard@gmail.com> wrote:
>> reverse that change and pass &gdesc.front() instead
>>
>> (and &pdesc.front())
>
> actually, &*gdesc.begin() may be better.

My recollection (of the standard) is that if the
vector is empty, neither begin() nor front() are
defined and/or can be dereferenced.  And these
are cases where the vector is empty.

But I would certainly defer to someone who
knows the standards better than I.
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: regression with 7f00642b [ In reply to ]
On Sat, May 12, 2018 at 12:17 AM, Jean-Yves Avenard <jyavenard@gmail.com> wrote:

> As it's called with length set to 0, it is fine.

Based on your far greater c++ expertise, the follow
is the next (final?) proposed patch:


{{{
diff --git a/mythtv/libs/libmythtv/mpeg/mpegtables.cpp
b/mythtv/libs/libmythtv/mpeg/mpegtables.cpp
index 3120b821f6..e61a699075 100644
--- a/mythtv/libs/libmythtv/mpeg/mpegtables.cpp
+++ b/mythtv/libs/libmythtv/mpeg/mpegtables.cpp
@@ -445,7 +445,7 @@ ProgramMapTable* ProgramMapTable::Create(
uint len = global_desc[i][1] + 2;
gdesc.insert(gdesc.end(), global_desc[i], global_desc[i] + len);
}
- pmt->SetProgramInfo(&gdesc[0], gdesc.size());
+ pmt->SetProgramInfo(gdesc.data(), gdesc.size());

for (uint i = 0; i < count; i++)
{
@@ -457,7 +457,7 @@ ProgramMapTable* ProgramMapTable::Create(
prog_desc[i][j], prog_desc[i][j] + len);
}

- pmt->AppendStream(pids[i], types[i], &pdesc[0], pdesc.size());
+ pmt->AppendStream(pids[i], types[i], pdesc.data(), pdesc.size());
}
pmt->Finalize();

}}}


And while it is not strictly necessary, I would
also propose this patch, in order to avoid
a potential future static analysis warnings:

{{{
diff --git a/mythtv/libs/libmythbase/mythsocket.cpp
b/mythtv/libs/libmythbase/mythsocket.cpp
index bed0172cad..66a2d3f675 100644
--- a/mythtv/libs/libmythbase/mythsocket.cpp
+++ b/mythtv/libs/libmythbase/mythsocket.cpp
@@ -1006,7 +1006,7 @@ void MythSocket::ResetReal(void)
if (avail)
{
trash.resize(max((uint)trash.size(),avail));
- m_tcpSocket->read(&trash[0], avail);
+ m_tcpSocket->read(trash.data(), avail);
}

LOG(VB_NETWORK, LOG_INFO, LOC + "Reset() " +
}}}



I have been running my BE and FE with these patches
and they seem to work for me.
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org