Added edl/comskip support over HTSP.

[kendrak24]

The possibility to skip commercials when streaming a recording over htsp (e.g. from XBMC) has been missing, but now that XBMC (Gotham) supports it, I've added the necessary htsp API changes. It supports two formats of commercial skip data: Comskip and EDL, in that priority order. The code first checks for the presence of a comskip file, and if that isn't found it looks for an edl file. I've tested this first on a modified 3.2 clone, and now on master.
To use it with XBMC the addon needs to implement the GetRecordingEdl method. I've forked the opdenkapm/xbmc.pvr-addons addon repo to https://github.com/kendrak24/xbmc-pvr-addons (branch hts.pvr_edl) and implemented it there.
This is the first github thing I've done, so I hope everything is correct. Let me know if there is something I need to change...

[adamsutton]

@kendrak24 some minor points:

1. Change the API name to something a bit more generic, i.e. don't reference a particular implementation EDL/comskip. Though not sure what it should be, I'll think about it some more.
2. Don't include EDL/comskip processing in the HTSP code, that's messy/ugly.
3. I don't like passing all these args with fixed length array spec, not necessary and makes the code ugly.
4. The HTSP API shouldn't be based in any way on files, i.e. the input ID param should be the DVR entry. Eventually when we come to sort out the DVR code we'll be able to better build the cutpoint stuff into the DVR APIs.
5. HTSP tends to pass things like your type field as a string, though I'm sure that's not always the case, so I might be persuaded to ignore that one.
6. I think the file processing could be cleaner, but that's not that critical as long as the internal APIs are correct.

I'd generally like to see very little code in HTSP for supporting this. Move all the file processing etc.. out to a file in the dvr/ directory. Have some API like:

dvr_cutpoint_list_t *dvr_get_cutlist ( u32 id );

i.e. it takes the DVR entry ID and returns a list of cut points. With a structures something like:

typedef struct dvr_cutpoint {
LIST_ENTRY(dvr_cutpoint) dc_link;
uint64_t dc_start; // not sure of units
uint64_t dc_stop; // not sure of units
enum {
DVR_CP_MUTE,
DVR_CP_CUT,
DVR_CP_SCENE,
DVR_CP_COMM
} dc_type;
} dvr_cutpoint_t;

typedef LIST_HEAD(,dvr_cutpoint) dvr_cutpoint_list_t;

[kendrak24]

For point 3, what do you suggest? Allocating dynamically and passing pointers, or using lists, or what?

[adamsutton]

@kendrak24 I would probably be more dynamic (lists) generally, but that can wait, as long as the HTSP API (and code) are clean I'll be happy at this stage.

But I was more specifically referring to including the constant array lengths in the function params, just pass ** and assume fixed length (if that's what's always defined). Yes strictly you lose some compiler checking, but the code doesn't look so messy. And better still just have a char **, where the last entry is a NULL (to detect end of the list).

But sort out the HTSP stuff first and get the code cleaned up.

[kendrak24]

OK, code cleaned up a bit. Moved processing out of htsp to new file under dvr as suggested.

[adamsutton]

Drop this field, you can infer it from the presence (or not) of cutpoints.

[adamsutton]

@kendrak24 I definitely want to get this included. but have lost track of where things were. I'm sorry its taken so long to get back to master dev. But I will try and review this one later this week.

[kendrak24]

@adamsutton OK. Let me know if there's anything I need to do.

[kendrak24]

@adamsutton Any estimate when this might be included?

[adamsutton]

@kendrak24 really, sorry I've been so snowed under, but I'm going to review it now...

[adamsutton]

or tomorrow, promise. But I've skimmed it and ti looks good, my main concern is the HTSP stuff, since once we do that we kinda have to stick to it. And I'm happy with that.

I don't really care that much about the EDL parsing bit, but I do just want to give it a quick scan for obvious errors before I merge.

[adamsutton]

this leaks on error below. Might also be better to have some sort of table driven approach to parsing:

static struct {
const char *ext;
void parse ( const char *path, cut_list_t *cuts );
} cut_parsers[] = {
{ .ext = ".edl". .parse = dvr_parse_edl },
{ .ext = ".txt". .parse = dvr_parse_comskip },
}

And just iterate over that list etc... That way adding a new parser for a new format will be a bit simpler. Needs some slight logic change and possibly simplification of the extension setup might be good.

Anyway that's just a thought

[adamsutton]

leak's the file descriptor, this sort of error is usually terminal anyway. but better to clean up and return error.

[adamsutton]

@kendrak24 lots of comments, it's a nice feature, pretty good code, so I was probably more picky than usual. But would like to see at least the style and memleaks fixed.

Changing to table driven parsers would be a nice to have.

[adamsutton]

@kendrak24 an progress on this? I would like to include as I think there are plenty of people that will benefit from this.

[kendrak24]

@adamsutton Yep, working on it. It'll probably be ready later today. Minus the table driven approach.

[adamsutton]

@kendrak24 can you rebase this to clean it up. I want to make a few changes before merge, but need to get out my laptop for that, so will do later.

[kendrak24]

Rebased and comments squashed.

[adamsutton]

@kendrak24 can you check the following branch https://github.com/tvheadend/tvheadend/tree/feature/dvr-cutpoint. I reworked the code a bit, can you check it still works and if you could fix any stupid mistakes I'd appreciate that!

[kendrak24]

@adamsutton Will try to check it tonight!

[kendrak24]

@adamsutton Sorry for this crude diff, but I didn't find a way to comment directly in your branch.

Found a couple of things, diff below comments.

1. In dvr_parse_comskip:
   The header parsing shouldn't return 0. That will cause a zero cutpoint to be inserted into the list.
   I changed it to return 2 if a header is found...
2. In dvr_parse_comskip:
   && should be || due to inverted logic.
3. struct dvr_cutpoint_parsers[];
   Leading dot needed for extension txt due to changed logic (or increase sptr by one...)
4. struct dvr_cutpoint_parsers[];
   Leading dot needed for extension edl due to changed logic (or increase sptr by one...)
5. In dvr_get_cutpoint_list:
   Only cleanup if no file was found, i.e. if 'i' == array size (I changed to >= but == should do...)

After these changes it worked fine with a comskip file for me.

---

diff --git a/src/dvr/dvr_cutpoints.c b/src/dvr/dvr_cutpoints.c
index 4f97b53..94197c9 100644
--- a/src/dvr/dvr_cutpoints.c
+++ b/src/dvr/dvr_cutpoints.c
@@ -113,11 +113,11 @@ dvr_parse_comskip
     if (*frame_rate <= 0.0)
       return 1;
     *frame_rate /= (*frame_rate > 1000.0f ? 100.0f : 1.0f);
-    return 0;
+    return 2;
   }

   /* Invalid line */
-  if(*frame_rate <= 0.0f && sscanf(line, "%d\t%d", &start, &end) != 2)
+  if(*frame_rate <= 0.0f || sscanf(line, "%d\t%d", &start, &end) != 2)
     return 1;

   /* Sanity Checks */
@@ -193,12 +193,12 @@ static struct {
   void       *opaque;
 } dvr_cutpoint_parsers[] = {
   {
-    .ext    = "txt",
+    .ext    = ".txt",
     .parse  = dvr_parse_file,
     .opaque = dvr_parse_comskip,
   },
   {
-    .ext    = "edl",
+    .ext    = ".edl",
     .parse  = dvr_parse_file,
     .opaque = dvr_parse_edl,
   },
@@ -252,7 +252,7 @@ dvr_get_cutpoint_list (uint32_t dvr_entry_id)
   }

   /* Cleanup */
-  if (i < ARRAY_SIZE(dvr_cutpoint_parsers)) {
+  if (i >= ARRAY_SIZE(dvr_cutpoint_parsers)) {
     dvr_cutpoint_list_destroy(cuts);
     return NULL;
   }