[Bug 693000] codecparsers: mpeg2: fix default quantizer matrix for intra blocks

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=693000
  GStreamer | gst-plugins-bad | git

--- Comment #16 from Gwenole Beauchesne <gb.devel at gmail.com> 2013-02-06 12:44:08 UTC ---
(In reply to comment #15)
> (From update of attachment 235201 [details])
> This looks fine in principle, but I would like to bikeshed a bit about the
> function naming / argument order:
> 
> In general the name should match the argument order as far as that's possible,
> so e.g. zigzag_to_raster (in_zigzag, out_raster) or _get_raster_from_zigzag
> (out_raster, in_zigzag).

OK, I just tried to figure out some existing practice from grep -r _to_
/usr/include/gstreamer-*/ :)

> Also, the 'object' being manipulated should ideally be at the front of the
> name, so gst_mpeg_video_quant_matrix_xyz(), even if it's not a real object or
> struct.
> 
> I think it should either be:
> 
> gst_mpeg_video_quant_matrix_zigzag_to_raster (const guint8 quant_zigzag[64],
> guint8 quant_raster[64]);
> 
> or the other way round with _from_*. Or just _{to,from}_{zigzag,raster} even.

It's hard to tell. There seems to be almost as many references to the _to_*
form as to the _from_* form.

For the codecparsers, it seems we usually put output args at first, so I will
probably just opt for gst_mpeg_video_quant_matrix_raster_from_zigzag() then. Is
that OK with you?

> Do we need the to_zigzag/from_raster variant at all, or is it merely for
> completeness?

I personally don't need it, it was just for completeness and testing we have
the identity operation: parse (in zigzag) -> helper to convert to raster ->
helper to convert back to zigzag + check we get the same matrix.

> Also, please add the bugzilla link to the bottom of the commit message.

Sure.

-- 
Configure bugmail: https://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.