Hey all,
TLDR: Fixed a bug with RDAirplay's Refresh Log function, patch attached.
Are any devs reading these? My last two patches seem to have disappeared
without remark, and one of them fixes a serious bug and probable
security hole.
Detail: The problem was the Refresh Log button (or automated function,
if you have that enabled) sometimes omitting or relocating events from
the refreshed log. Particularly troublingly for me, it would sometimes
relocate the CHAIN TO event from the end of the log, leading to
automated playout stalling at midnight.
The problem was that the refresh function uses log lines' unique IDs to
determine when lines in the current and modified log correspond, BUT the
current log could contain events that were active when it was started,
either due to a chain event or manual log loading, and those events
belong to a different log or a different instance of this one and may
have clashing IDs.
For example, suppose I have a log with a single event with ID '1'. If I
set it playing, then load a different log using the Select Log button,
the playing event remains in the active log and keeps its ID, but the
new log very probably has its own ID '1' since they are only unique per
log. Thus the current log now has two events with the same "unique" ID.
Call these events with equal ID the "clashing events".
This ID duplication could also happen due to a CHAIN TO that had a SEGUE
transition (as for logs generated with RDLogManager), as RDAirplay would
load the log without waiting for all of the previous day's events to end
first.
The problem then manifests in two ways upon a Refresh Log command:
1. If one of the clashing events had finished, but the other was still
outstanding, the second copy would be omitted during the refresh as it
would appear to be a future event here, but in the refreshed log it
would be tagged as finished or past.
2. Whenever a future event was being (re-)added during the refresh
process, the ID of the log line before it was used to try to place it
correctly. When duplicate IDs existed, this would place it after the
*first* clashing event, which is a holdover from a past log, rather than
the *last* one as intended.
Since the final event in the log was the CHAIN TO and we were trying to
place it after its predecessor, the final event that clashed after it
was held over from the previous day's log, the CHAIN TO ended up being
transplanted early in the running order, behind the currently playing
track, causing it never to execute and playout to stall.
My fix adds a new attribute to RDLogLine called isHoldover, to mark log
events that don't belong to this log as recorded in the database and
whose IDs may clash. I then amend RDLogEvent::logLineById and
RDLogEvent::lineById with an ignore_holdovers parameter that disregards
these special events when scanning for a match. At the same time I fix
the cleanup routine that precedes log loading, as an off-by-one error
led to it holding over events behind the currently playing one.
I find that this fixes may particular problem, but I suspect there are
more ID clash problems: for example, what if the user has added an event
to this log using RDAirplay's ADD button, whose ID clashes with one
mentioned in the database? The first phase of Refresh will mistake it
for a new event introduced using the Log Editor. Basically we need to
look over other uses of the ...byId functions to figure out whether
holdover or manually added events will trip them up.
Chris
diff '--exclude=*.m4' '--exclude=config*' '--exclude=autom4te*'
'--exclude=Makefile.in' -ur rivendell-2.8.1-base/lib/rdlog_event.cpp
rivendell-2.8.1-hacked/lib/rdlog_event.cpp
--- rivendell-2.8.1-base/lib/rdlog_event.cpp 2014-03-25 17:24:32.910970374
+0000
+++ rivendell-2.8.1-hacked/lib/rdlog_event.cpp 2014-03-25 17:17:34.891673861
+0000
@@ -974,20 +974,22 @@
}
-RDLogLine *RDLogEvent::loglineById(int id) const
+RDLogLine *RDLogEvent::loglineById(int id, bool ignore_holdovers) const
{
- for(int i=0;i<size();i++) {
- if(log_line[i]->id()==id) {
- return log_line[i];
- }
- }
- return NULL;
+ int line = lineById(id, ignore_holdovers);
+ if(line == -1)
+ return NULL;
+ else
+ return log_line[line];
}
-int RDLogEvent::lineById(int id) const
+int RDLogEvent::lineById(int id, bool ignore_holdovers) const
{
for(int i=0;i<size();i++) {
+ if(ignore_holdovers && log_line[i]->isHoldover()) {
+ continue;
+ }
if(log_line[i]->id()==id) {
return i;
}
diff '--exclude=*.m4' '--exclude=config*' '--exclude=autom4te*'
'--exclude=Makefile.in' -ur rivendell-2.8.1-base/lib/rdlog_event.h
rivendell-2.8.1-hacked/lib/rdlog_event.h
--- rivendell-2.8.1-base/lib/rdlog_event.h 2014-03-25 17:24:32.770970373
+0000
+++ rivendell-2.8.1-hacked/lib/rdlog_event.h 2014-03-25 17:17:34.379929858
+0000
@@ -60,8 +60,8 @@
QTime blockStartTime(int line);
RDLogLine *logLine(int line) const;
void setLogLine(int line,RDLogLine *ll);
- RDLogLine *loglineById(int id) const;
- int lineById(int id) const;
+ RDLogLine *loglineById(int id, bool ignore_holdovers=false) const;
+ int lineById(int id, bool ignore_holdovers=false) const;
int lineByStartHour(int hour,RDLogLine::StartTimeType type) const;
int lineByStartHour(int hour) const;
int nextTimeStart(QTime after);
diff '--exclude=*.m4' '--exclude=config*' '--exclude=autom4te*'
'--exclude=Makefile.in' -ur rivendell-2.8.1-base/lib/rdlog_line.cpp
rivendell-2.8.1-hacked/lib/rdlog_line.cpp
--- rivendell-2.8.1-base/lib/rdlog_line.cpp 2014-03-25 17:24:32.950970376
+0000
+++ rivendell-2.8.1-hacked/lib/rdlog_line.cpp 2014-03-25 17:17:35.427405865
+0000
@@ -173,6 +173,7 @@
log_link_id=-1;
log_link_embedded=false;
log_start_source=RDLogLine::StartUnknown;
+ is_holdover = false;
}
@@ -2075,3 +2076,13 @@
}
return QObject::tr("Unknown");
}
+
+bool RDLogLine::isHoldover() const
+{
+ return is_holdover;
+}
+
+void RDLogLine::setHoldover(bool b)
+{
+ is_holdover = b;
+}
diff '--exclude=*.m4' '--exclude=config*' '--exclude=autom4te*'
'--exclude=Makefile.in' -ur rivendell-2.8.1-base/lib/rdlog_line.h
rivendell-2.8.1-hacked/lib/rdlog_line.h
--- rivendell-2.8.1-base/lib/rdlog_line.h 2014-03-25 17:24:32.794970375
+0000
+++ rivendell-2.8.1-hacked/lib/rdlog_line.h 2014-03-25 17:17:33.844197854
+0000
@@ -263,6 +263,8 @@
static QString typeText(RDLogLine::Type type);
static QString timeTypeText(RDLogLine::TimeType type);
static QString sourceText(RDLogLine::Source src);
+ bool isHoldover() const;
+ void setHoldover(bool);
private:
int log_id;
@@ -365,6 +367,7 @@
int log_link_end_slop;
int log_link_id;
bool log_link_embedded;
+ bool is_holdover;
};
diff '--exclude=*.m4' '--exclude=config*' '--exclude=autom4te*'
'--exclude=Makefile.in' -ur rivendell-2.8.1-base/rdairplay/log_play.cpp
rivendell-2.8.1-hacked/rdairplay/log_play.cpp
--- rivendell-2.8.1-base/rdairplay/log_play.cpp 2014-03-25 17:24:32.546970372
+0000
+++ rivendell-2.8.1-hacked/rdairplay/log_play.cpp 2014-03-25
17:34:15.430973878 +0000
@@ -508,7 +508,7 @@
if(lines[running-1]<(size()-1)) {
remove(lines[running-1]+1,size()-lines[running-1]-1,false);
}
- for(int i=running-2;i>0;i--) {
+ for(int i=running-1;i>0;i--) {
remove(lines[i-1]+1,lines[i]-lines[i-1]-1,false);
}
if(lines[0]!=0) {
@@ -516,6 +516,12 @@
}
}
+ // Note that events left in the log are holdovers from a previous log.
+ // Their IDs may clash with those of events in the log we will now load,
+ // and it may be appropriate to ignore them in that case.
+ for(int i = 0, ilim = size(); i != ilim; ++i)
+ logLine(i)->setHoldover(true);
+
//
// Load Events
//
@@ -568,6 +574,7 @@
int current_id=-1;
int lines[TRANSPORT_QUANTITY];
int running;
+ int first_non_holdover = 0;
if(play_macro_running) {
play_refresh_pending=true;
@@ -613,7 +620,12 @@
for(int i=0;i<size();i++) {
d=logLine(i);
if(d->status()!=RDLogLine::Scheduled) {
- if((s=e->loglineById(d->id()))!=NULL) {
+ if((!d->isHoldover()) && (s=e->loglineById(d->id()))!=NULL) {
+ // A holdover event may be finished or active,
+ // but should not supress the addition of an
+ // event with the same ID in this log.
+ // Incrementing its ID here may flag it as an orphan
+ // to be removed in step 4.
s->incrementPass();
}
d->incrementPass();
@@ -629,6 +641,15 @@
}
}
+ // Find first non-holdover event, where start-of-log
+ // new events should be added:
+ for(int i=0;i<e->size();i++) {
+ if(logLine(i)->isHoldover())
+ ++first_non_holdover;
+ else
+ break;
+ }
+
//
// Pass 3: Add New Events
//
@@ -636,15 +657,15 @@
s=e->logLine(i);
if(s->pass()==0) {
if((prev_line=(i-1))<0) { // First Event
- insert(0,s,false,true);
+ insert(first_non_holdover,s,false,true);
}
else {
prev_id=e->logLine(prev_line)->id();
- insert(lineById(prev_id)+1,s,false,true);
+ insert(lineById(prev_id, /*ignore_holdovers=*/true)+1,s,false,true);
}
}
else {
- loglineById(s->id())->incrementPass();
+ loglineById(s->id(), /*ignore_holdovers=*/true)->incrementPass();
}
}
@@ -661,13 +682,16 @@
//
// Restore Next Event
//
- if(current_id!=-1 && e->loglineById(current_id)!=NULL) { //Make Next
after currently playing cart
- if((next_line=lineById(current_id))>=0) {
+ if(current_id!=-1 && e->loglineById(current_id)!=NULL) {
+ // Make Next after currently playing cart
+ // The next event cannot have been a holdover,
+ // as holdovers are always either active or finished.
+ if((next_line=lineById(current_id, /*ignore_holdovers=*/true))>=0) {
makeNext(next_line+1,false);
}
}
else {
- if((next_line=lineById(next_id))>=0) {
+ if((next_line=lineById(next_id, /*ignore_holdovers=*/true))>=0) {
makeNext(next_line,false);
}
}
_______________________________________________
Rivendell-dev mailing list
[email protected]
http://caspian.paravelsystems.com/mailman/listinfo/rivendell-dev
