Memory Corruption Makes Me Sad

Nothing makes programmers cower in fear more than memory corruption. These bugs are almost always A) fatal and B) ridiculously difficult to track down and C) hard to reproduce consistently. The combination of these three things can make you start thinking of your memory in surprisingly literal ways:

(and literal in more ways than one, since your memory is "trashed" hahahaha, oh that was bad...)


This particular blog posting is about my own struggle with a bug that I knew about for no less than four months, and for the last two weeks I worked on it 24/7. I fixed it, but it was very difficult to isolate.

Memory corruption happens when "the contents of a memory location are unintentionally modified due to programming errors. When the corrupted memory contents are used later in the computer program, it leads either to program crash or to strange and bizarre program behavior." What makes memory corruption particularly insidious is your program doesn't crash or behave strangely at the time of modification--it happens later, when something attempts to use the memory that was sullied.

Causes of memory corruption include use of uninitialized memory, use of unowned memory, buffer overflows, faulty heap memory management and multithreading problems. In all cases, the defining characteristic is where the program crashes isn't necessarily related to what went wrong.

Here's the best strategy I've found for dealing with memory corruption:

1. If you're really, really lucky, you might be able to catch the problem with something like DevPartner or Valgrind, but typically these only show the problem as it's blowing up. In the case of my bug, DevPartner ended up being beneficial only because it made my error more repeatable. Also, have a look at Application Verifier.
2. Get a static code analysis tool. If you're using VS.NET 2008, try using their Code Analysis tool. It is surprisingly effective at locating buffer overruns and errant pointer access. These tools may or may not help you, but they certainly can't hurt. If you are lucky, this may be all you need to find your problem. If it doesn't work, then you get to go on to the brute force methods (lucky you!)
   
3. Go through all classes and make sure every member variable is initialized correctly. Value types should be set to sensible values, pointer types should be initialized to NULL.
4. Search for all new/delete/malloc/free statements.  Ensure that all pointer values that are allocated begin their life as NULL. Ensure that the memory being released is immediately NULL'd. Ensure you have no new/free or malloc/delete mismatches. Ensure you do not free/delete memory twice. Ensure that any memory allocated by a third party library is also destroyed by that library (i.e. do not call "CrazyLibraryMemAlloc" and use free/delete to clean up unless you are positive that is the correct thing to do). Make sure your destructors and cleanup methods release all memory and NULL everything. Make sure you're using delete[] if the type was allocated with new []. In essence, everything should begin its life as NULL and end its life as NULL. This is probably the single best thing you can do to isolate memory tom-foolery.
   
5. Review every memset, memcpy and mem-whatever in the program (ditto for Win32 variants like CopyMemory). If you are using raw buffer pointers (e.g. void*, int*, etc.), consider wrapping them in something like QByteArray. Review any and all string handling code (in particular, strcpy and the likes). If you have any raw pointer string types, consider replacing them with a decent string class like CString or Qt's QString.
   
6. Are you using threads? Does the crash happen in a shared object? If so, this strongly implies your locking strategy (or lack thereof--even if you have locks, be absolutely certain they are working as you expect).
7. Determine if you are experiencing corruption in the same location, or if it's more random. If it's random corruption, then it is more likely to be a buffer overflow. If it's localized corruption (i.e. let's say the crash always happens in a shared queue, or in the same place in code), then it is more likely that code touching the shared item is invalid. If it crashes in the exact same place always, then you are in luck--you should be able to watch that location in a debugger and break on any read/write. Whether or not you have crashes in the same place is a huge, huge clue about your problem. Track this information religiously.
   
8. One method for determining if you have local/random corruption is to declare "no man's land" buffers directly above and below the item being corrupted. Like, nice, big 10k buffers that are initialized to "0xDEADBEEFDEADBEEFDEADBEEF..." When your program crashes, inspect those buffers. If those buffers contain invalid data, then it is not localized corruption. If they aren't corrupted, but the data structure they wrap is, then that strongly implies something that touches the sandwiched object is where the problem may lie.
9. It is not likely to be the C-runtime, third party code, obscure linking issues, etc. Think about it: you're not the only person using these libraries and tools. They are generally more thoroughly vetted because of Linus' law. Is it possible? Yeah, sure, anything's possible. But is it likely? Not really.
10. Unless evidence strongly implies otherwise, assume the issue is in your code. This is good, because it means it's something you can potentially fix. Otherwise, you may have to start a support case with whomever owns the code. If it's an open source project, you might get a quick response (or possibly no response at all...). If it's somebody like MSFT, it is going to take weeks at a minimum. Only as a last resort should you assume it's somewhere else, and be certain you have Real Information™ to backup your theory.
    

It may take a couple of people a day or five to go through the program and make all these changes depending on how big the application is, but it's generally the only real way to isolate problems. And it also gets you in the habit of being religiously fanatical about default values, pointer checking and correct deletion of objects, which is good.

For me, the issue ended up being extremely subtle (it evaded two separate code reviews by my peers), and after finding it, painfully obvious and somewhat embarrassing. I was having localized corruption around a shared queue that two threads accessed. The culprit was invalid locking code I'd written. Once the queue became corrupted, it would fail somewhere in the bowels of whatever queue object I'd been using (I tried CAtlList, QList, etc, but it didn't matter because none of them are thread-safe).

Which brings me back to item #10 in the above list: it's always your fault. It was my fault. It can be very tempting to assume otherwise, but generally I don't find this to be the case. So keep an open mind, think analytically, write down what you know and what you don't know, and you'll be done with the bug sooner than you know it!

DMOs Considered Harmful

Lately I've been working with DMO filters. Typically I've written regular DirectShow filters derived from the base classes, but after reading this article I decided that maybe it was time to start writing DMOs instead of transform filters. The advantages seemed attractive, and being able to write a filter that would work in Media Foundation was tempting.

After writing two of them, I can now say this was a bad idea. There are serious performance implications, major limitations and many of the proposed advantages are simply not true.

Let's start with the deal-breaker for me: you may run into serious performance issues when using a DMO filter in DirectShow because you cannot set the number or size of output buffers. When using the DMO Wrapper Filter (think of this filter as the "translator" between the DMO model and DShow filters--you could write your own DMO wrapper if you wanted), the ALLOCATOR_PROPERTIES on the output pin will always look something like:
You will always get one, big buffer for the output allocator. There is no way to change this that I know of. The article on MSDN mentions "...DMOs have no control over the number of buffers or memory allocators," but what they omit to tell you is that this can have serious consequences for high-performance playback, and particularly so for variable rate playback.

The same filter implemented as a typical transform filter allows me to specify the size and count of the output buffers:
Here, I made the DShow filter use 20 output buffers. This filter, with the exact same decoder, would do up to 8x on my machine without missing a beat. I could not reliably do better than 2x with the DMO filter before the filter graph manager reverted my SetRate() call. In case it isn't obvious, you give up a lot of control by using a DMO in DirectShow. And for some scenarios, it's pretty clear you sacrifice a lot of performance as well.

But it gets worse. Let's say your H264 encoder pukes everywhere. Vomits ALL OVER. You, being the responsible programmer you are, would like to communicate this failure to some higher power so it can come in with scrubbies and bleach and all that good stuff and clean up the technicolor yawn that is your filter innards.

Normally you'd call IMediaEventSink->Notify() and be done with it--the event gets handled asynchronously by the filter graph manager so you need not worry about threading issues (woe is the DShow coder who fires a synchronous event from their streaming thread), and everything can be dealt with in a common event handler. But someone, in their infinite wisdom, did not provide a standard eventing method for a DMO filter. Which means: your events fall on the floor.

There are a few options to deal with this. You can do what normal DirectShow filters do: hold a weak reference to IMediaEventSink and send events through that. But this requires a custom interface, and suddenly your filter is no longer that nice, clean abstraction that works in Media Foundation and DirectShow. You could create your own eventing interface, but it would need to be asynchronous (since the DMO is being called on the streaming thread) so this isn't exactly trivial. These options are not appealing.

These are the two major grievances I have with DMOs. Minor grievances include:

• The documentation mentions issues with IDMOQualityControl, which is the DMO version of IQualityControl. Purportedly, the DMO interface "...is not ideal for quality control due to the limitations of the interface itself." But nowhere are these "limitations" outlined. It'd be great if MSDN would make it clear what they are.
• The claim that "DMOs require less methods to implement, and they provide a documented API" is total nonsense. My DMO implementation was about ~300 lines and included no fewer than 14 methods I had to implement (note the section at the bottom outlining required methods). For CTransformFilter? 200 lines of code and 5 methods. Also, note that CTransformFilter is completely documented.
• Want to manually instantiate a filter? Good luck. Have fun. You basically have to know all sorts of complicated junk about apartment threading and COM and ATL to get this to work. It's possible, but it's a lot more work and not as well documented as manually instantiating a regular DirectShow filter.

I should be fair: some of these problems are really issues in the DMO Wrapper Filter and there's certainly nothing stopping someone from writing their own wrapper filter. But some of the issues such as the lack of eventing is not so easy to deal with. Regardless, the big question is: why bother? Why not just write a regular transform filter and not deal with any of these problems?

In retrospect, I'd be more inclined to devote my efforts to some cross-platform rendering solution, like GStreamer.

A fellow poster on a forum I frequent often says to gravity skeptics (yes, sadly people like this exist) "well, if in doubt, jump off a cliff." Someone commented that there wasn't such a succinct retort for evolution skeptics, but I think there is:



...all the intelligent design advocates die; settles that debate, I believe.

H264 media subtypes in Directshow

The media subtypes supported by Directshow are outlined in an MSDN article. Although the article lists five separate types, there are really only two distinct types:

1. MEDIASUBTYPE_AVC1: h.264 bitstream without start codes, and
2. MEDIASUBTYE_H264: h.264 bitstream with start codes

Basically, a subtype in directshow communicates the type of media that a filter outputs, so these types let connecting filters know that a filter outputs some variant of H.264. However, both of these types are--to differing degrees--nonsensical. Microsoft could have done a better job defining these types.

For AVC1, the actual sample data is prefixed with the length in big-endian format, which is completely redundant with DirectShow's IMediaSample implementation. IMediaSample already implements GetActualDataLength() to communicate the length of the buffer, so this additional data in the sample buffer is completely unnecessary and one source of error when writing a filter. You often see code that resembles:

IMediaSample pSample;
BYTE* pBuffer;
pSample->GetPointer(&pBuffer);
DoSomethingWithMyH264Data(pBuffer + 4,pSample->GetActualDataLength - 4));

The only possible benefit I can see to prefixing the length is it makes it easy to convert back to a type with the NALU start codes (0x00000001 can simply overwrite a 4-byte start size, for example, and voila: start codes), but that's really not a big deal.

The good thing about AVC1 is that the subtype defines a single NALU per sample buffer, which is great, because at least it makes this type somewhat compatible with the DirectShow architecture. Also good is the SPS/PPS info is communicated during pin negotiation, which is helpful when determining if your decoder is capable of handling the incoming H.264 video. (e.g. in the event of video a decoder cannot handle, you can not allow a pin connection in the first place instead of having to actually parse incoming video. Transform filters don't have to wait for SPS/PPS info to lumber along, which does not necessarily happen. And so on.)

I wouldn't say AVC1 is bad; I'd reserve that designation for the certifiably stupid H264 subtypes. The H264 subtypes define a raw H264 byte stream, which means they deliver NALUs delimited with the NALU start code (0x00000001 or 0x000001). Since the H264 subtypes do not specify one NALU per buffer, I assume this means a filter can potentially deliver multiple NALU per IMediaSample. People on the newsgroups even interpet this subtype to mean that "NALU boundaries don't even have to respect the sample boundaries," which means you could get a sample that didn't contain an entire NALU, or a sample that contained half of a NALU, and so on.

But this behavior is completely nonsensical from a timestamping perspective. Each IMediaSample has a timestamp, so this model effectively makes it impossible for a downstream filter to correctly timestamp incoming H.264, which really only makes this subtype feasible for live streaming, and you can pretty much forget about syncing with audio in any meaningful way.

Needless to say, this completely violates the IMediaSample abstraction. GetActualDataLength() is wrong in the same way AVC1 is wrong (the NALU start codes are present in the buffer), but so is GetMediaTime (time for which sample???), IsSyncPoint (precisely what NALU is a sync point?), IsDiscontinuity, etc. Of course, if you decide to make a filter that outputs the H264 subtype, you can always make it output a single NALU per IMediaSample buffer, but it doesn't change the fact that filters which accept H264 subtypes cannot depend on an IMediaSample being a discrete NALU, which makes interpreting the IMediaSample data problematic.

Furthermore, in the H264 subtypes, the SPS/PPS info is not communicated in the media format exchange, which means downstream filters cannot prepare their decoder until they have parsed incoming data and located the SPS/PPS info. This simply makes no sense. All proper RTP H.264 streams should communicate SPS/PPS info in the SDP exchange, and the info is shuffled away in the MP4 format because of how important it is for decoders to be able to quickly access it so decoding can begin immediately. To not have this info up front makes it far more difficult (although not impossible, by any means) to write a filter, and there is rarely a good reason to not provide it.

In any event, my advice: avoid the H264 subtypes, they are ridiculous and only make your filter a pain in the ass to deal with. The AVC1 format is better, but it could be improved by removing the superfluous size field from the media buffer.

Programming Fail: Directory.GetFiles()

I went to demo some shiny new code for a friend, and we both had a laugh when my program pretty much puked all over itself.

Admittedly I had not run this code on this particular machine before, and it was running Vista and .NET 3.5 (neither of which I'd tested against) But upon finding the bug, I find it difficult to understand what sort of design decision would involve such an arbitrary behavior.

Directory.GetFiles() seems like a pretty straight-forward function. You hand it a directory to search, and a search pattern (e.g. "*.exe"), and it returns a list of all the files in that directory which match the search. OK, I can handle that. Or so I thought.

The fine print of the documentation contains the gotcha:

The following list shows the behavior of different lengths for the searchPattern parameter:

* "*.abc" returns files having an extension of .abc, .abcd, .abcde, .abcdef, and so on.
* "*.abcd" returns only files having an extension of .abcd.
* "*.abcde" returns only files having an extension of .abcde.
* "*.abcdef" returns only files having an extension of .abcdef.

Ummm. OK. So, MSFT, basically your search pattern violates decades of common convention with regards to regular expressions, and the developer gets to figure this out when the app bombs? OK, sure. Brilliant. Ship it, yo.

Let's say you're looking for TIFF files, which can have either "tif" or "tiff" as the file extension. What happens is: calling GetFiles() with both of those extensions will result in the "tiff" files being added twice. Bzzt, fail MSFT, fail.

So, the fix is: either go through the aggregate list and remove duplicates, or remove the "*.tiff" search pattern (by the way, I got my supported extensions through the image handling classes in .NET). But it'd be a lot easier to simply have this method behave in a manner that is normal and predictable and sane: if I ask for "*.tif," I only want "*.tif". But I guess that's asking for too much.

More fun with AM_MEDIA_TYPE

While messing around with CMediaType (a wrapper class for AM_MEDIA_TYPE), I came across an inconsistency/bug. If you execute the following code:

 
 pmt->cbFormat = sizeof(WAVEFORMATEX);
 WAVEFORMATEX *wfex = (WAVEFORMATEX*)pmt->AllocFormatBuffer(sizeof(WAVEFORMATEX));

...you will find that wfex (which is a pointer to pbFormat) is NULL. My first thought was "out of memory?!" and the next thought was that simply wasn't possible. This call was about 30k lines of code deep in my application, so clearly memory allocation would have nailed me long before this point in time. So I poked around in mtype.cpp, which is the file that implements CMediaSource, and found the bug:

// allocate length bytes for the format and return a read/write pointer
// If we cannot allocate the new block of memory we return NULL leaving
// the original block of memory untouched (as does ReallocFormatBuffer)
BYTE*
CMediaType::AllocFormatBuffer(ULONG length)
{
    ASSERT(length);

    // do the types have the same buffer size

    if (cbFormat == length) {
        return pbFormat;
    }

    // allocate the new format buffer

    BYTE *pNewFormat = (PBYTE)CoTaskMemAlloc(length);
    if (pNewFormat == NULL) {
        if (length <= cbFormat) return pbFormat; //reuse the old block anyway.
        return NULL;
    }

    // delete the old format

    if (cbFormat != 0) {
        ASSERT(pbFormat);
        CoTaskMemFree((PVOID)pbFormat);
    }

    cbFormat = length;
    pbFormat = pNewFormat;
    return pbFormat;
}

It becomes pretty clear what the issue is: if the requested length is the same as cbFormat, then it simply returns pbFormat, but since pbFormat was never allocated, it simply returns 0x00000000. Bzzt.

I guess the obvious moral of this story is: when using wrapper classes, it's probably not a good idea to manually set the underlying structure parameters. But still, this is a clear bug: this method will CoTaskMemFree pbFormat if it already exists. So this class should probably be like so:

    if (pbFormat != NULL && cbFormat == length) {
        return pbFormat;
    }

If someone is allocating the same amount of memory, and pbFormat isn't null, it's acceptable to return pbFormat.

Again, I realize it doesn't make sense to set cbFormat if AllocFormatBuffer is going to do it for you, but that argument assumes one is aware that AllocFormatBuffer is going to do that for you. Nowhere in the documentation does it mention that I am forbidden from interacting with the underlying structure, so the only way you figure this out is by kicking yourself in the teeth, which I'd rather not do because it hurts.

Lastly, the same issue is present if the allocation itself fails. So a better implementation of the method is:

// allocate length bytes for the format and return a read/write pointer
// If we cannot allocate the new block of memory we return NULL leaving
// the original block of memory untouched (as does ReallocFormatBuffer)
BYTE*
CMediaType::AllocFormatBuffer(ULONG length)
{
    ASSERT(length);

    // do the types have the same buffer size, _and_ 
    // do we have a valid pbFormat pointer???
    if (pbFormat != NULL && cbFormat == length) {
        return pbFormat;
    }

    // allocate the new format buffer
    BYTE *pNewFormat = (PBYTE)CoTaskMemAlloc(length);
    if (pNewFormat == NULL) {
        // If the current pbFormat works, reuse it.  Otherwise, fail.
        return (pbFormat != NULL && length <= cbFormat) ?
            pbFormat : NULL;
    }

    // delete the old format
    if (pbFormat != NULL && cbFormat != 0) {
        CoTaskMemFree((PVOID)pbFormat);
    }

    cbFormat = length;
    pbFormat = pNewFormat;
    return pbFormat;
}

Slightly better, but I still hate AM_MEDIA_TYPE.

HD Video Standard

Despite the last couple of years being a time when "high definition" video has really gained traction, there's one surprising thing about HD video: it doesn't have an obvious definition. Dan Rayburn brings up this observation in a recent blog post:

For an entire industry that defines itself based on the word "quality", today there is still no agreed upon standard for what classifies HD quality video on the web....If the industry wants to progress with HD quality video, we're going to have to agree on a standard - and fast.

He's absolutely right. Many companies attempt to pass off 480p as HD video, but most video enthusiasts would reject such an assertion--after all, if it isn't HD for an analog signal, why would it be HD for a digital signal? Likewise, lots of video is encoded at an unacceptably low bit rate which results in obvious artifacts. Why would such poor quality video be considered "high definition?"

Wikipedia's definition for High-definition television is a decent start:

High-definition television (or HDTV) is a digital television broadcasting system with higher resolution than traditional television systems (standard-definition TV, or SDTV). HDTV is digitally broadcast; the earliest implementations used analog broadcasting, but today digital television (DTV) signals are used, requiring less bandwidth due to digital video compression.

This is still lacking. What exactly is "higher resolution than traditional television systems?" And just what SDTV system, since there were many of them? And is resolution all there is to it? What if I encode video to 1080p but at a horrible bit rate, which causes lots of blocking artifacts? What about video with an odd aspect ratio, where the number of verticals lines doesn't pass muster? Clearly this definition is lacking.

Some aspects of creating a standard are fairly straight-forward: most people seem to be fairly comfortable with 720p being the "minimum" resolution at which video can be encoded to. Ben Waggoner had an interesting proposal where 720p was acceptable, but it was also acceptable to generalize it to anything with "at least 16 million pixels per second," which takes into account both framerate and resolution. He also brought up the issue of using horizontal resolution as a criteria, since not everything is 16:9.

But on the question of "quality," most simply punted, and I find this odd. Ben Waggoner mentions:

Hassan Wharton-Ali brought up another good point on the thread - HD should actually be HD quality. It can’t be a lousy, over-quantized encode using a suboptimally high resolution just so it can be called HD.

A good test is the video should look worse (due to less detail), not better (due to less artifacts), if encoded at a lower resolution at the same data rate. If reducing your frame size makes the video look better when scaled to the same size, then the frame size is too high!

It is a good point, and I don't completely disagree with Ben's proposal--it should look worse due to less detail if encoded at a lower resolution. But this is the crux of the issue: what does it mean to look worse? Is this just a subjective judgment call on behalf of the person encoding the video? I don't think this addresses the problem of having a minimum acceptable "quality" for HD video.

Dan Rayburn's suggestion is even less desirable, in my opinion:

To me, the term HD should refer to and be defined by the resolution and a minimum bitrate requirement. Since you could have a 1080p HD video encoded at a very low bitrate, which could result in a poor viewing experience inferior to that of a higher-bitrate video in SD resolution, the resolution and bitrate is the only way to define HD.

The first issue with this is the "minimum bit rate" requirement would have to somehow scale with the resolution and frame rate. It would have to account for the codec being used. This would result in an impossibly complicated system, endless arguments, etc. (for example, would we impose the same bit rate requirement on H.264 as we would on VC1? What about "future" codecs?)

A bigger issue is not all video content is the same. The resulting "quality" of a video encoded to a given bitrate absolutely has a relationship with the video being encoded. A video with very little movement can often be encoded with a low bit rate and look fantastic, so the bit rate requirement would essentially amount to wasted bandwidth. Conversely, a video with a lot of motion and scene changes may require a lot more bits to get an acceptable, block-free viewing experience--and it's not clear what that acceptable threshold would be.

I find both of these suggestions insufficient. I propose an alternative: objective video quality algorithms. The idea is straight-forward: by comparing the source material with the output material, we can objectively establish a score that at least has some meaningful relationship with Mean Opinion Scores. In a nutshell, a MOS is how "good" the average person thinks some piece of video appears.

Peak Signal-to-Noise/Mean Squared Error is the most common algorithm, albeit one that is quite crude, widely considered to be deficient by most engineers and scientists. But it's 2009, baby--we can do better. We have better.

My suggestion would be the Structural SIMilarity Index, which is relatively inexpensive (its closely related brother, MSSIM, is much more pricey) and definitely correlates better with MOS.

How would this work?

1. During the encode process, a SSIM score is computed for each frame using the input as a reference image.
2. This process is repeated for every input frame, and every output frame.
3. The lowest observed SSIM score is the resulting quality score for that piece of encoded video. (I suppose another alternative is to use the average. Yet another option is to use the variance. I'd avoid the median, since it's robust against outliers, and outliers matter)
   
4. If the lowest observed SSIM score is less than some threshold, then the video cannot be considered High Definition.
   

For a visual representation of how this works, take a look at this graph:


This is a graph of SSIM over time, displaying multiple bit rates. The x-axis is frame number, and the y-axis is SSIM score. My input was a VGA, 30 FPS, ~30 second raw-RGB video clip. Each line corresponds with a bit rate requested of the encoder (x264's H.264 implementation, using a baseline profile--you can see this by the low SSIM scores at the beginning of the video due to single-pass encoding). Notice the clear relationship between SSIM scores and bit rate. Also note how much variance there is in video quality: clearly certain portions of this clip are "more difficult" to encode, and this results in a degradation of video quality. Also, notice a clear law of diminishing returns: as more and more bits are thrown at the video clip, the SSIM scores converge on 1.0--SSIM at 2 mbit/sec aren't substantially different from the SSIM scores at 500 kbit/sec.

There are a few gotchas with this plan: what if we're changing the frame rate (e.g. 3:2 pulldown) and there is no clear reference frame to which we compare the output? How do we determine the SSIM threshold? Do we really want to use SSIM, or is some other algorithm better?

The first question is answered relatively easy: we compare only what was input to the encoder and the resulting output. Presumably the process of manipulating the frame rate is separate from the process of encoding. What we're talking about is how well of a job our encoder does matching the input.

The second question is easier, but it requires someone conducting subjective video quality assessment tests to determine what threshold corresponds with a baseline SSIM number. In effect, someone has to do some statistical analysis on data captured during viewing sessions of actual people watching actual footage encoded with an actual compression algorithm, and determine a threshold that correlates well with people's perception of "High Definition." But at least with SSIM, this is a manageable process: once a threshold is determined, it's really independent of a whole slew of factors, like codec, the video being encoded, etc.

Let's say we decide that any SSIM score below 0.9 invalidates the video from being called "high definition"--for the above video, this would mean 500 kbps would be just slightly too poor to call HD (notice the poor quality at the beginning of the clip). And 1000 kbps would be more than acceptable.

Lastly, even though PSNR is an outdated method, I see no reason a high-definition standard could not include metrics for both objective quality tests. There are other objective video quality algorithms, and certainly more will be developed in the future, so any standard should be open to extension at a later date.

(side note: maybe part of our problem is this emphasis on bit rate--which has no relationship to quality beyond "more is probably better"--when our real emphasis should be on a metric that correlates with quality, but I digress)

I don't really care what objective metric is used, and certainly there is plenty of debate over which objective method correlates best with MOS, and what threshold should be used--but let's at least be scientific about this. If there's going to be a "standard" for High Quality video, then let's choose a standard that will carry us forward and not create a quagmire.