GNOME Bugzilla – Bug 327028
MS Video 1 palettized AVI doesn't work
Last modified: 2006-02-24 16:36:32 UTC
I have some AVI files in MS Video 1 format, with 256 colours palette. Curently GStreamer shows a load of wierd colours instead of the image, because it does not read the palette. This patch adds support for those files. The modification to ffmpegcolorspace.c is necessary because, without that, I am getting negotiation errors: something like gstbasetransform.c(1245):subclass did not specify output size. The file is 160x120 with a 256-colour palette Apparently the MS Video 1 decoder gives back a buffer of 20224 bytes (160*120*1 for the image + 256*4 for the palette), but gst_ffmpegcsp_get_unit_size returned 19200 (160*120*1, no palette). I haven't experienced any regressions with this modification, but it should be tested.
Created attachment 57379 [details] [review] Patch for palettized MS Video 1 AVI files
I noticed #132341 now: actually the patch causes a regression on the AVI file attached to it. Maybe the best thing is to add a boolean parameter to the caps, saying if a palette is present. If it is not, gst_ffmpegcsp_get_unit_size should subtract 256 * 4 from the size, otherwise it should not.
Here is a revised patch, which works with both the MS Video 1 file and bb28.avi. It is necessary to apply the gst-ffmpeg patch as well
Created attachment 57383 [details] [review] Patch for gst-plugins-base (revised)
Created attachment 57384 [details] [review] Patch for gst-ffmpeg, needed to support palettized MS Video 1 AVI files
Can you please post a file? (attach it here if it's small or just post a link..) I think #331790 should be fixed before committing this patch, if you want to take a look.. :)) ciao
I'm not entirely convinced about the whole palette_in_frame thing. Are we doing this anywhere else (ie. having the palette data as part of GST_BUFFER_DATA)? Why not just update the caps you set on each outgoing buffer with a new "palette" entry as required, basically like we do already now? That way everything would be handled automagically, no? Luca: I've got a test file - ping me on IRC if you want it.
Tim, you are probably mixing two different problems here. The addition of the "palette_data" caps is done by the line + palette = strf_data; Later on, the (already existing) code checks if the palette pointer is not null, and, in that case, creates the "palette_data" caps. /* palette */ if (palette && GST_BUFFER_SIZE (palette) >= 256 * 4) { GstBuffer *copy = gst_buffer_copy (palette); [...] gst_caps_set_simple (caps, "palette_data", GST_TYPE_BUFFER, copy, NULL); The justification for "palette_in_frame" is different. ffmpeg's decoders put the palette as part of their output buffer, independent of GStreamer. The result is, in gst-ffmpeg the resulting GST_BUFFER_DATA has the palette appended to it. GStreamer decodres don't. This is reflected in gstffmpegcolorspace: ffmpeg's avpicture_get_size returns the size of the buffer in bytes, including the palette bytes, just as every buffer output by a ffmpeg decoder. gstffmpegcolorspace deals with GStreamer buffers, which do not have those bytes. Therefore, it has to correct it by subtracting the number of palette bytes. The "palette_in_frame" caps (which is bool) set to TRUE just means: this buffer was produced by an ffmpeg decoder, so the number of bytes computed by avpicture_get_size is right and does not have to be corrected. An alternative to the "palette_in_frame" caps would be: - gst-ffmpeg trims the GST_BUFFER_DATA by removing the palette bytes added by ffmpeg (which are totally unnecessary, since the palette is already in the "palette" caps) - gstffmpegcolorspace trims the size, unconditionally, as it does now
Should be fixed now, many thanks! 2006-02-24 Tim-Philipp Müller <tim at centricular dot net> * gst-libs/gst/riff/riff-media.c: (gst_riff_create_video_caps): Pick up palette for MS video v1 (#327028, patch by: Fabrizio Gennari <fabrizio dot get at tiscali dot it>) 2006-02-24 Tim-Philipp Müller <tim at centricular dot net> * ext/ffmpeg/gstffmpegcodecmap.c: (gst_ffmpeg_get_palette), (gst_ffmpeg_set_palette): Use AVPALETTE_SIZE macro instead of magic value for clarity. * ext/ffmpeg/gstffmpegdec.c: (gst_ffmpegdec_frame): In GStreamer, the size of the palette is not part of GST_BUFFER_SIZE, so adjust buffer size of outgoing buffers accordingly if there's a palette (fixes #327028, based on patch by: Fabrizio Gennari).