[gstreamer-bugs] [Bug 442034] [avi] add support for subtitle streams (GAB2)

[GStreamer (bugzilla.gnome.org)]

If you have any questions why you received this email, please see the text at
the end of this email. Replies to this email are NOT read, please see the text
at the end of this email. You can add comments to this bug at:
  http://bugzilla.gnome.org/show_bug.cgi?id=442034

  GStreamer | gst-plugins-good | Ver: HEAD CVS

Tim-Philipp Müller changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |t.i.m at zen.co.uk
 Attachment #100954|none                        |needs-work
               Flag|                            |




------- Comment #6 from Tim-Philipp Müller  2007-12-14 18:01 UTC -------
Cool stuff!  Looks mostly fine, but here are the usual nitpicks:

gst_avi_subtitle_chain:

    - one shouldn't use g_assert() to check input really; it's more meant
      to be used as a guard to ensure internal consistency (make sure
      internal decision-making or calculations ended up as they should,
      or maybe to clarify implied preconditions in certain bits of code
      to make it easier to grok). I you don't get as much data as you
      expect to, you could either return FLOW_UNEXPECTED or a
      goto broken_gap2_chunk; where you post a STREAM DECODE error
      message on the bus and return FLOW_ERROR (I'd do the latter).

    - if you check for ASCII characters, it increases the readability
      of the code if you use e.g. 'A' rather than 0x41 (or even
      memcmp or strncmp in places that are not particularly performance
      critical)

    - should use GST_WARNING_OBJECT to print debug lines for broken
      bitstream (IMHO)

    - if the bitstream is broken, post a STREAM DECODE error on the bus
      using the GST_ELEMENT_ERROR macro and return FLOW_ERROR.  Most
      GStreamer code puts error goto labels at the end of the chain
      function for this kind of stuff FWIW :)

    - you don't consistently check whether you're reading beyond the
      size of the buffer (esp. with name_length, but also with the
      other reads)

    - all variables need to be declared at the beginning of a block
      (this is re. the 'file' subbuffer)

    - gst_element_get_pad() returns a reference which you're leaking here

    - you should always pass the flow return from gst_pad_push()
      back upstream

gst_avi_subtitle_class_init:

    - the boilerplate macro already does the parent_class stuff for you

gst_avi_subtitle_init:

    - never put code that does something into g_assert() or g_return_if_fail()
      or the like statements, otherwise those function calls will just
      disappear if someone compiles the plugin against a GLib with
      assertions disabled.

    - save the pads you create in the element structure (and use that
      in your chain function instead of the _get_pad)

    - use GST_DEBUG_FUNCPTR(gst_avi_subtitle_chain) in _set_chain_function

header:

    - not used: GstAviSubtitlePrivate
    - not used: GstAviSubtitlesubtitle_header_name
    - not used: GstAviSubtitlesubtitle_file


As for the BOM:

     - yes, ideally subparse should recognise that and convert its
       input on-the-fly as approriate (I think there's even a bug
       open for this)

     - if you don't want to fix subparse right now, you can of
       course just parse the BOM yourself and convert the output to
       UTF-8 yourself as required, and then push that down to
       subparse.

      - what to do with the subtitle name depends a bit on what it
        contains; we could just ignore it for now, or post it on
        the bus (but then the app might mistake that as the main
        title of the movie or so, that wouldn't be good either).

Last question:

     - does this work right with seeking yet?


-- 
See http://bugzilla.gnome.org/page.cgi?id=email.html for more info about why you received
this email, why you can't respond via email, how to stop receiving
emails (or reduce the number you receive), and how to contact someone
if you are having problems with the system.

You can add comments to this bug at http://bugzilla.gnome.org/show_bug.cgi?id=442034.