[issue1717] Patch for pgssubdec to support RLE data over multiple packets
hugohuetzel hugo.huet...@noid-project.de added the comment: rle_picture_len: length of rle data needed for decoding the subpicture (over all packets) rle_data_len: length of rle data received Any suggestions for an other variable name? _ FFmpeg issue tracker iss...@roundup.ffmpeg.org https://roundup.ffmpeg.org/roundup/ffmpeg/issue1717 _
[issue1717] Patch for pgssubdec to support RLE data over multiple packets
hugohuetzel hugo.huet...@noid-project.de added the comment: sounds like a seperate bugfix belonging to a seperate patch It is not possible to append data to a too big buffer. So the 2 patches belongs together. these names are poor, its quite unlcear what they represent I have choosen a name equal to the names that was in the code. The new varibale rle_picture_len contains the length of the picture rle picture data specified in the packet header. I don't understand what should be unclear with this. Feel free to change it. _ FFmpeg issue tracker iss...@roundup.ffmpeg.org https://roundup.ffmpeg.org/roundup/ffmpeg/issue1717 _
[issue1717] Patch for pgssubdec to support RLE data over multiple packets
Michael Niedermayer michae...@gmx.at added the comment: On Mon, Feb 01, 2010 at 09:16:34AM +, hugohuetzel wrote: hugohuetzel hugo.huet...@noid-project.de added the comment: sounds like a seperate bugfix belonging to a seperate patch It is not possible to append data to a too big buffer. So the 2 patches belongs together. these names are poor, its quite unlcear what they represent I have choosen a name equal to the names that was in the code. The new varibale rle_picture_len contains understand what should be unclear with this. that Feel free to change it. the chances of that happening are very low, iam far too busy [...] _ FFmpeg issue tracker iss...@roundup.ffmpeg.org https://roundup.ffmpeg.org/roundup/ffmpeg/issue1717 _
[issue1717] Patch for pgssubdec to support RLE data over multiple packets
Michael Niedermayer michae...@gmx.at added the comment: On Mon, Feb 01, 2010 at 01:39:12PM +, Michael Niedermayer wrote: Michael Niedermayer michae...@gmx.at added the comment: On Mon, Feb 01, 2010 at 09:16:34AM +, hugohuetzel wrote: hugohuetzel hugo.huet...@noid-project.de added the comment: sounds like a seperate bugfix belonging to a seperate patch It is not possible to append data to a too big buffer. So the 2 patches belongs together. these names are poor, its quite unlcear what they represent I have choosen a name equal to the names that was in the code. The new varibale rle_picture_len contains roundup does not like my way of underlineing it seems i meant that:picture rle picture data, this makes no sense [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The real ebay dictionary, page 2 100% positive feedback - All either got their money back or didnt complain Best seller ever, very honest - Seller refunded buyer after failed scam _ FFmpeg issue tracker iss...@roundup.ffmpeg.org https://roundup.ffmpeg.org/roundup/ffmpeg/issue1717 _
[issue1717] Patch for pgssubdec to support RLE data over multiple packets
New submission from hugohuetzel hugo.huet...@noid-project.de:
Patch for pgssubdec to support RLE data over multiple packets
--
files: pgs_multipck.patch
messages: 9092
priority: normal
status: new
substatus: new
title: Patch for pgssubdec to support RLE data over multiple packets
type: patch
_
FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/roundup/ffmpeg/issue1717
_--- ffmpeg_org/libavcodec/pgssubdec.c 2009-09-05 21:59:23.0 +0200
+++ ffmpeg/libavcodec/pgssubdec.c 2010-01-28 12:05:01.0 +0100
@@ -53,7 +53,7 @@
int w;
int h;
uint8_t *rle;
-unsigned int rle_buffer_size, rle_data_len;
+unsigned int rle_buffer_size, rle_data_len, rle_picture_len;
} PGSSubPicture;
typedef struct PGSSubContext {
@@ -149,7 +149,6 @@
* @param avctx contains the current codec context
* @param buf pointer to the packet to process
* @param buf_size size of packet to process
- * @todo TODO: Enable support for RLE data over multiple packets
*/
static int parse_picture_segment(AVCodecContext *avctx,
const uint8_t *buf, int buf_size)
@@ -166,42 +165,59 @@
sequence_desc = bytestream_get_byte(buf);
if (!(sequence_desc 0x80)) {
-av_log(avctx, AV_LOG_ERROR, Decoder does not support object data over multiple packets.\n);
-return -1;
-}
+if (buf_size-4 ctx-picture.rle_picture_len-ctx-picture.rle_data_len) {
+av_log(avctx, AV_LOG_ERROR, Too much RLE data for specified length of %d.\n, ctx-picture.rle_picture_len);
+goto fail;
+}
-/* Decode rle bitmap length */
-rle_bitmap_len = bytestream_get_be24(buf);
+av_fast_malloc(ctx-picture.rle, ctx-picture.rle_buffer_size, ctx-picture.rle_data_len+buf_size-4);
-/* Check to ensure we have enough data for rle_bitmap_length if just a single packet */
-if (rle_bitmap_len buf_size - 7) {
-av_log(avctx, AV_LOG_ERROR, Not enough RLE data for specified length of %d.\n, rle_bitmap_len);
-return -1;
-}
+if (!ctx-picture.rle)
+goto fail;
-ctx-picture.rle_data_len = rle_bitmap_len;
+memcpy(ctx-picture.rle+ctx-picture.rle_data_len, buf, buf_size-4);
-/* Get bitmap dimensions from data */
-width = bytestream_get_be16(buf);
-height = bytestream_get_be16(buf);
-
-/* Make sure the bitmap is not too large */
-if (ctx-presentation.video_w width || ctx-presentation.video_h height) {
-av_log(avctx, AV_LOG_ERROR, Bitmap dimensions larger then video.\n);
-return -1;
-}
+ctx-picture.rle_data_len += buf_size-4;
+} else {
+/* Decode rle bitmap length */
+rle_bitmap_len = bytestream_get_be24(buf);
+
+/* Check if we have to much data for rle_bitmap_length if just a single packet */
+if (rle_bitmap_len buf_size - 7) {
+ctx-picture.rle_data_len = rle_bitmap_len - 4;
+ctx-picture.rle_picture_len = rle_bitmap_len - 4;
+} else {
+ctx-picture.rle_data_len = buf_size - 11;
+ctx-picture.rle_picture_len = rle_bitmap_len - 4;
+}
-ctx-picture.w = width;
-ctx-picture.h = height;
+/* Get bitmap dimensions from data */
+width = bytestream_get_be16(buf);
+height = bytestream_get_be16(buf);
+
+/* Make sure the bitmap is not too large */
+if (ctx-presentation.video_w width || ctx-presentation.video_h height) {
+av_log(avctx, AV_LOG_ERROR, Bitmap dimensions larger then video.\n);
+goto fail;
+}
-av_fast_malloc(ctx-picture.rle, ctx-picture.rle_buffer_size, rle_bitmap_len);
+ctx-picture.w = width;
+ctx-picture.h = height;
-if (!ctx-picture.rle)
-return -1;
+av_fast_malloc(ctx-picture.rle, ctx-picture.rle_buffer_size, ctx-picture.rle_data_len);
-memcpy(ctx-picture.rle, buf, rle_bitmap_len);
+if (!ctx-picture.rle)
+goto fail;
+
+memcpy(ctx-picture.rle, buf, ctx-picture.rle_data_len);
+}
return 0;
+
+fail:
+ctx-picture.rle_picture_len = 0;
+ctx-picture.rle_data_len = 0;
+return -1;
}
/**
@@ -336,6 +352,7 @@
{
AVSubtitle*sub = data;
PGSSubContext *ctx = avctx-priv_data;
+intret = 0;
/*
* The end display time is a timeout value and is only reached
@@ -361,9 +378,11 @@
/* Process bitmap */
sub-rects[0]-pict.linesize[0] = ctx-picture.w;
-if (ctx-picture.rle)
-if(decode_rle(avctx, sub, ctx-picture.rle, ctx-picture.rle_data_len) 0)
-return 0;
+if ((!ctx-picture.rle) ||
+(ctx-picture.rle_data_len != ctx-picture.rle_picture_len) ||
+(ctx-picture.rle_picture_len == 0) ||
+(decode_rle(avctx, sub, ctx-picture.rle,
[issue1717] Patch for pgssubdec to support RLE data over multiple packets
Carl Eugen Hoyos ceho...@rainbow.studorg.tuwien.ac.at added the comment: Is there a sample available? -- status: new - open substatus: new - needs_more_info _ FFmpeg issue tracker iss...@roundup.ffmpeg.org https://roundup.ffmpeg.org/roundup/ffmpeg/issue1717 _
[issue1717] Patch for pgssubdec to support RLE data over multiple packets
Carl Eugen Hoyos ceho...@rainbow.studorg.tuwien.ac.at added the comment: Relevant sample uploaded to incoming/issue1717/pgs_multipck_2.m2ts @hugohuetzel: Your patch contains a mix of functional and cosmetic changes, which makes reviewing harder and is therefore not accepted here (=please resend without the re-indentation of rle_bitmap_len =, width = etc.) _ FFmpeg issue tracker iss...@roundup.ffmpeg.org https://roundup.ffmpeg.org/roundup/ffmpeg/issue1717 _
[issue1717] Patch for pgssubdec to support RLE data over multiple packets
hugohuetzel hugo.huet...@noid-project.de added the comment:
Sorry, but there are no cosmetic changes.
I have moved rle_bitmap_len =, width = etc. to a else-
multi-packet (if-block) or a single/first-packet (else-
But i agree with you, the patch looks very ugly. So here is a
new patch (with removed/changed else-block for easier
reading)
_
FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/roundup/ffmpeg/issue1717
_--- ffmpeg_org/libavcodec/pgssubdec.c 2009-08-26 13:31:37.0 +0200
+++ ffmpeg/libavcodec/pgssubdec.c 2010-01-28 20:15:27.349872999 +0100
@@ -53,7 +53,7 @@
int w;
int h;
uint8_t *rle;
-unsigned int rle_buffer_size, rle_data_len;
+unsigned int rle_buffer_size, rle_data_len, rle_picture_len;
} PGSSubPicture;
typedef struct PGSSubContext {
@@ -149,7 +149,6 @@
* @param avctx contains the current codec context
* @param buf pointer to the packet to process
* @param buf_size size of packet to process
- * @todo TODO: Enable support for RLE data over multiple packets
*/
static int parse_picture_segment(AVCodecContext *avctx,
const uint8_t *buf, int buf_size)
@@ -166,21 +165,35 @@
sequence_desc = bytestream_get_byte(buf);
if (!(sequence_desc 0x80)) {
-av_log(avctx, AV_LOG_ERROR, Decoder does not support object data over multiple packets.\n);
-return -1;
-}
+if (buf_size-4 ctx-picture.rle_picture_len-ctx-picture.rle_data_len) {
+av_log(avctx, AV_LOG_ERROR, Too much RLE data for specified length of %d.\n, ctx-picture.rle_picture_len);
+goto fail;
+}
+
+av_fast_malloc(ctx-picture.rle, ctx-picture.rle_buffer_size, ctx-picture.rle_data_len+buf_size-4);
+if (!ctx-picture.rle)
+goto fail;
+
+memcpy(ctx-picture.rle+ctx-picture.rle_data_len, buf, buf_size-4);
+
+ctx-picture.rle_data_len += buf_size-4;
+
+return 0;
+}
+
/* Decode rle bitmap length */
rle_bitmap_len = bytestream_get_be24(buf);
-/* Check to ensure we have enough data for rle_bitmap_length if just a single packet */
-if (rle_bitmap_len buf_size - 7) {
-av_log(avctx, AV_LOG_ERROR, Not enough RLE data for specified length of %d.\n, rle_bitmap_len);
-return -1;
+/* Check if we have to much data for rle_bitmap_length if just a single packet */
+if (rle_bitmap_len buf_size - 7) {
+ctx-picture.rle_data_len = rle_bitmap_len - 4;
+ctx-picture.rle_picture_len = rle_bitmap_len - 4;
+} else {
+ctx-picture.rle_data_len = buf_size - 11;
+ctx-picture.rle_picture_len = rle_bitmap_len - 4;
}
-ctx-picture.rle_data_len = rle_bitmap_len;
-
/* Get bitmap dimensions from data */
width = bytestream_get_be16(buf);
height = bytestream_get_be16(buf);
@@ -188,20 +201,25 @@
/* Make sure the bitmap is not too large */
if (ctx-presentation.video_w width || ctx-presentation.video_h height) {
av_log(avctx, AV_LOG_ERROR, Bitmap dimensions larger then video.\n);
-return -1;
+goto fail;
}
ctx-picture.w = width;
ctx-picture.h = height;
-av_fast_malloc(ctx-picture.rle, ctx-picture.rle_buffer_size, rle_bitmap_len);
+av_fast_malloc(ctx-picture.rle, ctx-picture.rle_buffer_size, ctx-picture.rle_data_len);
if (!ctx-picture.rle)
-return -1;
+goto fail;
-memcpy(ctx-picture.rle, buf, rle_bitmap_len);
+memcpy(ctx-picture.rle, buf, ctx-picture.rle_data_len);
return 0;
+
+fail:
+ctx-picture.rle_picture_len = 0;
+ctx-picture.rle_data_len = 0;
+return -1;
}
/**
@@ -336,6 +354,7 @@
{
AVSubtitle*sub = data;
PGSSubContext *ctx = avctx-priv_data;
+intret = 0;
/*
* The end display time is a timeout value and is only reached
@@ -361,9 +380,11 @@
/* Process bitmap */
sub-rects[0]-pict.linesize[0] = ctx-picture.w;
-if (ctx-picture.rle)
-if(decode_rle(avctx, sub, ctx-picture.rle, ctx-picture.rle_data_len) 0)
-return 0;
+if ((!ctx-picture.rle) ||
+(ctx-picture.rle_data_len != ctx-picture.rle_picture_len) ||
+(ctx-picture.rle_picture_len == 0) ||
+(decode_rle(avctx, sub, ctx-picture.rle, ctx-picture.rle_data_len) 0))
+goto end;
/* Allocate memory for colors */
sub-rects[0]-nb_colors= 256;
@@ -371,7 +392,13 @@
memcpy(sub-rects[0]-pict.data[1], ctx-clut, sub-rects[0]-nb_colors * sizeof(uint32_t));
-return 1;
+ret = 1;
+
+end:
+ctx-picture.rle_picture_len = 0;
+ctx-picture.rle_data_len = 0;
+
+return ret;
}
static int decode(AVCodecContext *avctx, void *data, int *data_size,
[issue1717] Patch for pgssubdec to support RLE data over multiple packets
hugohuetzel hugo.huet...@noid-project.de added the comment: Ok, something went wrong with the text. Next try :-) Sorry, but there are no cosmetic changes. I have moved rle_bitmap_len =, width = etc. to a else- single/first-packet (else-block) But i agree with you, the patch looks very ugly. So here is a new patch (with removed/changed else-block for easier reading) _ FFmpeg issue tracker iss...@roundup.ffmpeg.org https://roundup.ffmpeg.org/roundup/ffmpeg/issue1717 _
[issue1717] Patch for pgssubdec to support RLE data over multiple packets
hugohuetzel hugo.huet...@noid-project.de added the comment: Sorry, but there are no cosmetic changes. I have moved rle_bitmap_len =, width = etc. to a else-block. So there is either a multi-packet (if-block) or a single/first-packet (else-block) _ FFmpeg issue tracker iss...@roundup.ffmpeg.org https://roundup.ffmpeg.org/roundup/ffmpeg/issue1717 _
[issue1717] Patch for pgssubdec to support RLE data over multiple packets
hugohuetzel hugo.huet...@noid-project.de added the comment: But i agree with you, the patch looks very ugly. So here is a new patch (with removed/changed else-block for easier reading) _ FFmpeg issue tracker iss...@roundup.ffmpeg.org https://roundup.ffmpeg.org/roundup/ffmpeg/issue1717 _
[issue1717] Patch for pgssubdec to support RLE data over multiple packets
Michael Niedermayer michae...@gmx.at added the comment:
On Thu, Jan 28, 2010 at 07:40:43PM +, hugohuetzel wrote:
[...]
@@ -166,21 +165,35 @@
sequence_desc = bytestream_get_byte(buf);
if (!(sequence_desc 0x80)) {
-av_log(avctx, AV_LOG_ERROR, Decoder does not support object data
over multiple packets.\n);
-return -1;
-}
+if (buf_size-4
ctx-picture.rle_picture_len-ctx-picture.rle_data_len) {
+av_log(avctx, AV_LOG_ERROR, Too much RLE data for specified
length of %d.\n, ctx-picture.rle_picture_len);
+goto fail;
+}
+
+av_fast_malloc(ctx-picture.rle, ctx-picture.rle_buffer_size,
ctx-picture.rle_data_len+buf_size-4);
+if (!ctx-picture.rle)
+goto fail;
+
+memcpy(ctx-picture.rle+ctx-picture.rle_data_len, buf, buf_size-4);
you arent checking if buf_size = 4
+
+ctx-picture.rle_data_len += buf_size-4;
+
+return 0;
+}
+
trailing whitespace
/* Decode rle bitmap length */
rle_bitmap_len = bytestream_get_be24(buf);
-/* Check to ensure we have enough data for rle_bitmap_length if just a
single packet */
-if (rle_bitmap_len buf_size - 7) {
-av_log(avctx, AV_LOG_ERROR, Not enough RLE data for specified
length of %d.\n, rle_bitmap_len);
-return -1;
+/* Check if we have to much data for rle_bitmap_length if just a
single packet */
+if (rle_bitmap_len buf_size - 7) {
+ctx-picture.rle_data_len = rle_bitmap_len - 4;
+ctx-picture.rle_picture_len = rle_bitmap_len - 4;
+} else {
+ctx-picture.rle_data_len = buf_size - 11;
+ctx-picture.rle_picture_len = rle_bitmap_len - 4;
duplicate
}
-ctx-picture.rle_data_len = rle_bitmap_len;
why is this changed on both sides of the if() ?
[...]
@@ -361,9 +380,11 @@
/* Process bitmap */
sub-rects[0]-pict.linesize[0] = ctx-picture.w;
-if (ctx-picture.rle)
-if(decode_rle(avctx, sub, ctx-picture.rle,
ctx-picture.rle_data_len) 0)
-return 0;
+if ((!ctx-picture.rle) ||
+(ctx-picture.rle_data_len != ctx-picture.rle_picture_len) ||
+(ctx-picture.rle_picture_len == 0) ||
+(decode_rle(avctx, sub, ctx-picture.rle, ctx-picture.rle_data_len)
0))
+goto end;
too obfuskated
[...]
_
FFmpeg issue tracker iss...@roundup.ffmpeg.org
https://roundup.ffmpeg.org/roundup/ffmpeg/issue1717
_
