[gst-devel] typefinding

[Ronald Bultje]

Hi Benjamin,

I don't like the %-suggest stuff, but I'll accept it. ;). I'll try to do
some suggestions to general improvement of stuff. Most of this is minor
nagging, but simply needs to be done to make it even better.

On Mon, 2003-10-27 at 02:55, Benjamin Otte wrote:
> I have put every typefind function that doesn't require a lib (that is
> anything but ffmpeg) into a new plugin, so now typefinding doesn't load
> every plugin in the world anymore. That new plugin is located in
> gst/typefind/gsttypefindfunctions.c and shows how easy it is to write
> typefind functions now.

I guess that's good. Some comments:

static void
id3_type_find (GstTypeFind *tf, gpointer unused)
{
  /* detect ID3v2 first */
  guint8* data = gst_type_find_peek (tf, 0, 10);
  if (data) {
    /* detect valid header */
    if (memcmp (data, "ID3", 3) == 0 &&
	data[3] != 0xFF && data[4] != 0xFF &&
	(data[6] & 0x80) == 0 && (data[7] & 0x80) == 0 &&
	(data[8] & 0x80) == 0 && (data[9] & 0x80) == 0) {
      gst_type_find_suggest (tf, GST_TYPE_FIND_MAXIMUM, ID3_CAPS);
      return;
    }
  }
  data = gst_type_find_peek (tf, -128, 3);
  if (data && memcmp (data, "TAG", 3) == 0) {
    gst_type_find_suggest (tf, GST_TYPE_FIND_MAXIMUM, ID3_CAPS);
  }
}

I don't see why you're doing this. Shouldn't you do:

  /* detect ID3v2 first */
  guint8* data = gst_type_find_peek (tf, 0, 10);
  if (data) {
    /* detect valid header */
    if (memcmp (data, "ID3", 3) == 0 &&
	data[3] != 0xFF && data[4] != 0xFF &&
	(data[6] & 0x80) == 0 && (data[7] & 0x80) == 0 &&
	(data[8] & 0x80) == 0 && (data[9] & 0x80) == 0) {
      gst_type_find_suggest (tf, GST_TYPE_FIND_MAXIMUM, ID3_CAPS (2));
    } else if (memcmp (data, "TAG", 3) == 0) {
      gst_type_find_suggest (tf, GST_TYPE_FIND_MAXIMUM, ID3_CAPS (1));
    }
  }

SUrely, you need only 3 instead of ten bytes for the other ID3 header,
but I personally don't think we should worry about ten bytes. You could
even turn them around if you really want to. It looks simpler to me...

In bugzilla, there's a bug because of files that have an ID3v2 header at
offset 512 instead of at the beginning of the file. We need to support
this too, I'm affraid, so you might want to request a bigger buffer and
scan the whole buffer for the ID3v2 header... Yeah, this sucks, I know.
;).

Lastly, why is the offset for id3v1 -128 bytes?

#define WAV_CAPS GST_CAPS_NEW ("wav_type_find", "audio/x-wav", NULL)
static void
wav_type_find (GstTypeFind *tf, gpointer unused)
{
  guint8 *data = gst_type_find_peek (tf, 0, 4);

  if (data) {
    if (memcmp (data, "RIFF", 4) == 0) {
      data = gst_type_find_peek (tf, 8, 4);
      if (data) {
	if (memcmp (data, "WAVE", 4) == 0)
	  gst_type_find_suggest (tf, GST_TYPE_FIND_MAXIMUM, AVI_CAPS);
      } else {
	gst_type_find_suggest (tf, GST_TYPE_FIND_POSSIBLE, AVI_CAPS);
      }
    }
  }
}

This should be WAV_CAPS, not AVI_CAPS. Also, note that you see "RIFF" as
POSSIBLE (50). I personally see no reason to make this more than 10 or
even 1, since RIFF is a generic storage format. You might laugh at this
now, but back in college, I've used RIFF as a network data transport
protocol. ;). 50 is way too high, imo. Anyway, that's probably a
personal opinion... Fact is that if you fail to read the first 12 bytes,
something's terribly wrong. Don't be too positive on your chances to
play back anything then. ;).

Last point: matroska typefinding isn't in there. You probably want to
move that over, too. It doesn't depend on the lib. (Btw: matroska
typefinding is semi-broken, it only detects 50% of the files out there,
but I'll fix that soon... :) ).

Ronald

-- 
Ronald Bultje <rbultje at ronald.bitfreak.net>
Linux Video/Multimedia developer