GNOME Bugzilla – Bug 693000
codecparsers: mpeg2: fix scan order for in-stream quantization matrices
Last modified: 2013-02-06 17:09:44 UTC
Created attachment 234969 [details] [review] codecparsers: mpeg2: fix default quantizer matrix for intra blocks Quantizer matrices are encoded in zigzag scan order in the bitstream, but they are stored in raster scan order when they are parsed. So, default matrices shall also be prepared in raster scan order if the sequence_header() does not provide any. Otherwise, we had a mismatch between explicit matrices stored in raster scan order vs. default matrices that were prepared in zigzag scan order. There are two ways to fix this issue: (i) Specify that matrices are stored in zigzag-scan order ; (ii) Specify that matrices are stored in raster scan order. The attached patch implements (ii). Some people would prefer (i) because there is some existing practice, based on incorrect/unspecified behaviour... For sure, the current codecparser is a mix of (i) [default matrix] and (ii) [parsed].
I think I would finally opt for solution (i) instead due to existing practice in drivers and also because we just ask the parser to parse the source bitstream not to necessarily interpret results from it. So, the idea here is to just hand out the original QM matrices as is, i.e. in zigzag scan order, as in the original bitstream and let upper layers perform conversions, if necessary. I would prepare two patches, one for making sure all matrices remain in zigzag scan order, and another one to provide utility functions to convert from zigzag scan order to raster scan order. What do you think? Thanks.
I think it is better to keep the default matrix as it is (zig-zag order) and fix the mpegvideoparser as proposed in the patch: https://bugs.freedesktop.org/show_bug.cgi?id=58176 . Then provide the utility functions as you mentioned in comment 1.
(In reply to comment #2) > I think it is better to keep the default matrix as it is (zig-zag order) and > fix the mpegvideoparser as proposed in the patch: > https://bugs.freedesktop.org/show_bug.cgi?id=58176 . Then provide the utility > functions as you mentioned in comment 1. Could you be more specific, i.e. why you think it's better that way? There are a number of compelling reasons for either solution. I will wait for others to comment too and, by Tuesday, I will update the bug with the definitive way I would like to resolve this issue. I won't list the reasons now, as I don't want to influence the brainstorming here in either way. :)
As long as the encoded matrix is in zig-zag order, parser should parse the data as it is and we are not allowed to change the order..right?(Because AFAIK that is the duty of parser :))..I was not thinking about the driver , just taking it as a general case.
I'll prefer the zig-zag order for codec parser because from software side, to decode an mpeg2 data, people need zig-zag ordered QT to restore matrix to original data. That's why mpeg2 store QT in zig-zag order in data. If codecparser parse to raster-scan order, software still need change to zig-zag order to decoding file and this should be unnecessary.
Raster scan order is the natural order HW decoders expect. Besides, this is also the resulting order for SW decoding once 7.3.1 is applied. However, the role of the parser is to simply parse the original bitstream and it's up to upper layers to interpret the data. HW decoders expect raster scan order but an extra copy from the parser to the HW (I omit intermediate) is done anyway so, if needed, we can also convert the matrix on-the-fly. So, I have finally decided to implement option (i), while providing utility functions to perform the conversions. There is also an additional reason: it looks unlikely that some ancient drivers that are still used around get ever fixed, and we will probably have to stick to the existing behaviour where matrices are pushed to the driver in the original (zigzag scan order) form.
Created attachment 235200 [details] [review] codecparsers: mpeg2: store quantization matrices in zigzag scan order
Created attachment 235201 [details] [review] codecparsers: mpeg2: add helpers to convert quantization matrices
Isn't there a different zig zag pattern in some cases? (for interlaced content?)
(In reply to comment #9) > Isn't there a different zig zag pattern in some cases? (for interlaced > content?) No, not for MPEG-2 AFAIK. And, if there was, we did not handle that either anyway. :) I think I see what you mean, it doesn't look to be MPEG-2. 7.3.1 implies that only scan 0 is used. Probably MPEG-4 where, depending on the field parity, the scan could be going to the right first, or going to the bottom first. Or this is for something different to the quantization matrices.
http://www.bretl.com/mpeghtml/zigzag.HTM refers to this for mpeg-2, but it's of course far from authorative, or I might be misunderstanding the context. Just thought it might be relevant for deciding the API, since one would have to pick the right zigzag table for the conversion then, no?
(In reply to comment #11) > http://www.bretl.com/mpeghtml/zigzag.HTM refers to this for mpeg-2, but it's of > course far from authorative, or I might be misunderstanding the context. Just > thought it might be relevant for deciding the API, since one would have to pick > the right zigzag table for the conversion then, no? Ah, that's for the DCT coefficients. The quantisation matrices always use the scan 0 (zigzag) order. scan 0 == "alternate_scan set to zero". Quantization matrices are denoted W (weighted coefficients). The DCT coefficients are denoted QF. The output of the inverse quantisation process is a function of QF * W * something else (roughly). So yes, that represents different things. :)
yes, Gwenole is correct, xxx_quantiser_matrix is only encoded in zigzag order. Refer to 6.3.11 of ISO/IEC 13818-2 : 2000 (E) "intra_quantiser_matrix – This is a list of sixty-four 8-bit unsigned integers. The new values, encoded in the default zigzag scanning order as described in 7.3.1, replace the previous values."
Comment on attachment 235200 [details] [review] codecparsers: mpeg2: store quantization matrices in zigzag scan order Makes sense, IMHO. Please add the bugzilla link somewhere at the bottom of the commit message.
Comment on attachment 235201 [details] [review] codecparsers: mpeg2: add helpers to convert quantization matrices 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). 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. Do we need the to_zigzag/from_raster variant at all, or is it merely for completeness? Also, please add the bugzilla link to the bottom of the commit message.
(In reply to comment #15) > (From update of attachment 235201 [details] [review]) > 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.
Sure, thanks! I also noticed we managed to mix en_GB spelling (quantise*) with en_US spelling (quantize*) in the existing API here :)
Created attachment 235305 [details] [review] codecparsers: mpeg2: add helpers to convert quantization matrices Use gst_mpeg_video_quant_matrix_get_XXX_from_YYY() instead. I can drop the get_zigzag_from_raster() variant if needed.
(In reply to comment #17) > Sure, thanks! > > I also noticed we managed to mix en_GB spelling (quantise*) with en_US spelling > (quantize*) in the existing API here :) What do you mean/what do you want me to do? I can still see references to both *_quantiser_matrix (GstMpegVideoQuantMatrixExt) and *_quantizer_matrix (GstMpegVideoSequenceHdr). The problem with settling down to one spelling would imply an API change anyway. Do we want to do that, while we are at it?
Comment on attachment 235305 [details] [review] codecparsers: mpeg2: add helpers to convert quantization matrices Thanks!
I was just amused by it, don't know if we need to fix it. It's probably not worth it.
Thanks! commit 250555a7dec8f1a396a20994d28395249eb406d7 Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Tue Feb 5 11:56:46 2013 +0100 codecparsers: mpeg2: add helpers to convert quantization matrices. Add utility functions to convert quantization matrices from zigzag scan order (as encoded in the bitstream) into raster scan order. Also provide another function to reverse the operation. https://bugzilla.gnome.org/show_bug.cgi?id=693000 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit 46c01de05dbb50c74aca137298da5b3d16918034 Author: Cong Zhong <congx.zhong@intel.com> Date: Thu Jan 31 16:13:22 2013 +0800 codecparsers: mpeg2: store quantization matrices in zigzag scan order. Quantizer matrices are encoded in zigzag scan order in the bitstream, but they are stored in raster scan order when they are parsed. However, default matrices were also prepared in zigzag scan order, hence the mismatch. i.e. the matrices were presented either in raster scan order if they are explicitly present in the bitstream, or they were presented in zigzag scan order if the default definitions were to be used instead. One way to solve this problem is to always expose the quantization matrices in zigzag scan order, since this is the role of the parser to not build up stories from the source bitstream and just present what is in there. Utility functions will be provided to convert quantization matrices in either scan order. https://bugzilla.gnome.org/show_bug.cgi?id=693000 Signed-off-by: Cong Zhong <congx.zhong@intel.com> Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>