[gstreamer-bugs] [Bug 335635] Need an OGG retagging element

[GStreamer (bugzilla.gnome.org)]

Do not reply to this via email (we are currently unable to handle email
responses and they get discarded).  You can add comments to this bug at
http://bugzilla.gnome.org/show_bug.cgi?id=335635
 GStreamer | gst-plugins-bad | Ver: HEAD CVS


Tim-Philipp Müller changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #63535|none                        |commented-on
               Flag|                            |




------- Comment #17 from Tim-Philipp Müller  2006-05-17 08:46 UTC -------
Very clean and nice code. Looks generally fine to me.

Some random thoughts though:

 - is there a particular reason why you've decided to go for an element
   that processes and retags a raw vorbis stream rather than one that
   operates on a raw ogg stream? (both have advantages/disadvantages,
   I was just wondering what your reasons where or if this has been
   discussed on IRC or elsewhere before).

 - if the element is to be implemented as a vorbis stream retagger
   maybe it should derive from the already existing vorbisparse
   element? There are extra things to consider, like stream
   header packets being part of the caps etc.
   (apologies if all that came up before and was rejected as
   an idea, I haven't been following this discussion up until now)

 - in the current implementation in gst_vorbis_tag_chain() new_buf
   should probably be stamped with the same values for timestamp /
   duration / offset / offset_end etc. as the incoming vorbis
   comments packet buffer.


> What should the element do with tags sent down the pipeline? The docs don't
> seem to be clear on what to do, and what the merge-mode means, in a three-way
> merge of pipeline, internal, and application tags. Currently the element just
> ignore pipeline tags.

I think that's okay, after all you get the upstream tags as part of the stream
as a vorbis comments packet/header buffer. I'd just treat the existing tags as
they are in the vorbis comment buffer the same way tag events are usually
handled.


> What tests should be added for the element? A few possible ones spring to
> mind:
> a) tagging an untagged vorbis stream,
> b) removing tags from a tagged vorbis stream, and
> c) retagging and tagged vorbis stream
> 
> In each of the cases checking that the tags in the output stream are correct.

Sounds good.



-- 
Configure bugmail: http://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.