[mythtv] Re: [mythtv-commits] mythtv commits

[Chris Pinkham]

Better grab a cup of coffee if you plan on reading this. :)

>mythtv at cvs.mythtv.org said:
>
>> ----------------------------------------------------------------------------
>> Changes committed by cpinkham on Wed Apr 20 04:44:05 2005
>> * Commmercial Flagging changes:
>
>> - Rearrange some debugging statements.
>you changed
>
>"block %1 with length %2, which would put "
>"comm block length under minimum of "
>"%3 seconds.")

I changed it from this:

         QString("Ignoring what appears to be commercial "
                 "block %1 with length %2, which would put "
                 "comm block length under minimum of "
                 "%3 seconds.")
                 .arg(curBlock)
                 .arg(fbp->start - breakStart)
                 .arg(commDetectMinCommBreakLength));

To this:

        QString("Ignoring what appears to be commercial"
                " block at frame %1 with length %2, "
                "length of %3 frames would put comm "
                "block length under min of %4 seconds.")
                .arg(breakStart)
                .arg(fbp->start - breakStart)
                .arg(fbp->frames)
                .arg(commDetectMinCommBreakLength));


Now that I look at it again, I think neither is entirely
accurate.  The 'if' statement that this is part of the 'else' of
is this:

if ((breakStart >= 0) &&
    ((fbp->end - breakStart) > (MAX_COMM_BREAK_LENGTH * fps)))
{
    if (((fbp->start - breakStart) > (MIN_COMM_BREAK_LENGTH * fps)) || 
        (lastEnd == 0))
    {

The message is part of the else from that 2nd if.  They are both wrong
in saying that the next block appears to be a commercial block
because there is nothing that indicates that, we haven't even looked at
the score for the block.  The only thing we're basing the fact that
the next block is part of a commercial break or not is the length.

Something more correct is probably:

        QString("Ignoring blocks starting at frame %1 "
                "with length %2, length is under minimum "
                "comm block length of %3 seconds.")
                .arg(curBlock)
                .arg(fbp->start - breakStart)
                .arg(commDetectMinCommBreakLength));

Or something referencing the fact that that the next block would put us
over the max length but without that block we are under the minimum length.

>but this is actually correct for that part of the code
>trust me, i tested it, there's a reason i worded it that way.

It was the wording saying that the next block appeared to be a commercial
block that threw me and then I neglected to take that part out when I
cut-and-pasted part of that text from another section.

>that is part of the bugfix for flagging tiny commercials
>that were getting closed off if the length was less than max length
>without a checking against min length 
>
>iirc, it has to refer to the length of the previous block, not the next. 

Right, so it should be reworded to reflect the next block is too big to
be a commercial whether it looks like one or not because that is what we're
really checking.

>>   - Revert the change (temporarily for now) that made strict mode require
>>     blank frames to be totally blank so frames with logos on them but
>>     otherwise blank would not be detected as so in strict mode.
>
>:((
>I know you said temporarily, 
>but requiring blank frames to be totally blank works tons better (imho)

The reason I reverted this temporarily is because myself and others have
been seeing worse flagging since this went in.  I have had several people
tell me that the flagging is worse for them in 0.18 than it was in 0.17
and after I put this patch in, my flagging got worse as well.  It partially
has to do with the stations you record.  I record a lot of shows off of CBS
here in the U.S. and CBS has an annoying habit of wanting me to see their
logo as much as possible, so they display it on every blank frame and then
turn it off when the commercial starts.  As soon as the commercial ends,
and the show is about to begin, the next blank frame has their logo on it.
The WAF went way down and I can't have that. :)  I didn't want to have
people taking a step backwards so I reverted this change temporarily until
something else can be done.

I'm not 100% sure if this strictness is necessary if logo detection is working
well enough, but I know 100% for sure that it breaks commercial detection 
with strict turned ON for lots of people here in the U.S. that record
shows off certain stations.

I've hesitated in the past to add user-configurable settings to control
 the flagging criteria, but am thinking that this is one that I want to add.

>I know, "better" is subjuective

Right, that's the thing, you see the recordings you make, but since I've
been working on this code for a couple years now and people know that, 
someone (or someones) usually let me know if it's taken a turn for the
worse.  I'm not opposed to putting it back in, but I think it will be as
an option as you also propose later in your post.

>Here's some objective stats from 50 recordings on my drive (long):
>
>--for the 80% of shows that don't leave the logo up--
>    -it (0.18 with strict mode) detects 100% correctly 99% of the time!!!
>
>--for the 10% shows that leave it up at the start of a commercial--
>    -it still detects "100%" correctly 99% of the time
>     since the end is all that matters for a skip
>       -it just won't auto skip 
>       -you have to hit 'z'(once or twice)

'Twice' means it wasn't 100%. :)

I don't understand how you could have 100% accurate detection if the
stations really leave the logo up?  Since the blocks are based off of blank
frames, and the logo would cause the frames to not be detected as blank,
then you would not be getting the blocks defined that you need so flagging
would be off.  If this is working as you say it is, then I think the
stations you are recording off of must be displaying a true blank frame (ie,
no logo) before and after the commercial break, but that is not the case
for several stations I record off of, they put their logo on every frame
possible.

>--for the other 10% of shows that leave it up at the end-- 
>    -it detects 100% correctly only 5% of the time
>    -it comes back too early 90% of the time which is good
>     (because of the scenechange and logo detection routines)

And this is the case with some stations here, it comes back early
because the blank frame between commercials didn't have the logo.

I think if the logo detection is accurate, the need for this option
is lower.  If logo detection is working right, there shouldn't be (m)any
 frames falsely detected as blank just because there is something that
happens to be in that small logo area that isn't a logo while the rest of
the screen is dark.

>On the other hand,
>Not requiring blank frames to be totally blank gets lots of false positives

I'd be interested to see the frameInfo map for these cases, does it really
detect a lot of dark frames as blank because they are blank except for
some small object in the logo area?  That would be kind of odd.  Is this
MST3K with the robots in the bottom corner of the video? :)

>====
>ie according to my empirical observations
>(it's pretty sad how much time i spent on this - easily 8 hours)
>it's 18 times worse to allow logos on "blank frames"
>====

I'm not following this.  If the other 95% of the frame is blank and we're
just not checking the 5% that the logo should be in, how can it be causing
that many false positives?  This is almost the same as not checking the
border area.

>There has to be another way to deal with the few shows that leave the logo up.
>Breaking comm detection for 90% of the shows, 
>because 10% leave the logo up at the end of the commercial, 
>doesn't make sense to me.

I'm not seeing the results you are and others that have complained evidently
aren't either, so that's why I elected to revert the patch temporarily.

I appreciate the time you've spent in this and explaining things and think
that will be helpful, but I must not be seeing the false positive blank frames
to the extent you're seeing them.


>temporarily, maybe this should have its own setting
>instead of depending on the strict setting
>
>something like
>    blankCanHaveLogo =
>              gContext->GetNumSetting("BlankCanHaveLogo", 0);

I'm not opposed to something like this for now, but think we may be able
to work around it a better way (that won't require the end user to tune
anything).

This is in CVS now, you may have seen the commit before I finished typing
my thesis here.  (Undocumented) setting is "CommDetectBlankCanHaveLogo" and
 defaults to 1 for now to keep the functionality as just prior to 0.18.

Maybe the logo area needs to be tighter like a pixel mask, and not a box.  OR,
probably even better the code could check if a logo was present first then
if the logo was present, we ignore the logo area during blank-frame detection.
How does that sound?

>BUT
>===
>there's got to be a way to do a check for blank frames that have logos on them
>to see if the 60th frame before or after it, also has a logo on it, otherwise
>don't mark it as blank.

I think that's a possiblity, but we'd need to check more than one frame
since the logo detection isn't quite accurate enough right now.  Lucas is
looking into better logo detection I believe, so maybe we'll see something
on that front in a little while.  I gave him some code where I played
with Sobel detection.  The initial detection was working great, it was
the frame-by-frame detection that was taking a while, then I abandoned it
to get the semi-realtime stuff coded.

Another way to do this might be to have 2 flags per frame while scanning
it.  One flag would indicate the logo area was blank/non-blank and the
other would indicate the non-logo area was blank/non-blank.  If the
non-logo area was blank and we think there's a logo on the frame, then
we could check X frames before and after the frame we think is blank.  If
the frames before have a logo and the ones after don't (or vice-versa)
then that tells us we are on a transition frame between commercial and
non-commercial and we mark the frame as blank.  This could all be done
at the beginning of the post-analysis phase since we'd have access to
the 2 "blank" flags as well as the logo information for all frames.
This is more complicated than the existing method but should be a lot
more accurate.

I didn't commit a patch to change the wording on the message at the top
of the email, I figured I'd get your opinion on that before I committed
anything.

-- 
Chris