[FFmpeg-devel] [PATCH] DVBSUBTILES, fixes to decoding/encoding and a new way of burning the subtitles onto the screen

[Michael Niedermayer]

On Mon, Sep 26, 2011 at 11:43:10PM +0100, JULIAN GARDNER wrote:
> 
> 
> 
> 
> 
> >________________________________
> >From: Michael Niedermayer <michaelni at gmx.at>
> >To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> >Sent: Monday, 26 September 2011, 23:25
> >Subject: Re: [FFmpeg-devel] [PATCH] DVBSUBTILES, fixes to decoding/encoding and a new way of burning the subtitles onto the screen
> >
> >On Sun, Sep 25, 2011 at 12:18:20AM +0200, Clément Bœsch wrote:
> >> From: joolzg <joolzg at btinternet.com>
> >> 
> >> ---
> >> Here is the second patch from Julian Gardner.
> >> 
> >> I did my best to remove the unrelated changes, fix the style and various other
> >> things, but it's still far from perfect. Some real reviews are needed.
> >
> >Before i can review this, i first need an explanation of what these
> >changes are supposed to do.
> >like:
> >The changes in file1 are needed to fix X implement Y and you can test
> >this with file Z command line C
> >... file2 ...
> >...
> >
> >A properly split patchset would be better than such list of course
> >
> >
> >[...]
> >> @@ -162,6 +162,21 @@ static uint8_t *audio_out;
> >>  static unsigned int allocated_audio_out_size, allocated_audio_buf_size;
> >>  
> >>  static short *samples;
> >> +
> >> +static AVBitStreamFilterContext *video_bitstream_filters;
> >> +static AVBitStreamFilterContext *audio_bitstream_filters;
> >> +static AVBitStreamFilterContext *subtitle_bitstream_filters;
> >
> >unused
> >
> >
> >[...]
> >> @@ -1024,17 +1069,22 @@ static void dvbsub_parse_region_segment(AVCodecContext *avctx,
> >>          region = av_mallocz(sizeof(DVBSubRegion));
> >>  
> >>          region->id = region_id;
> >> +        region->version = -1;
> >>  
> >>          region->next = ctx->region_list;
> >>          ctx->region_list = region;
> >>      }
> >>  
> >> +    version = ((*buf)>>4) & 15;
> >>      fill = ((*buf++) >> 3) & 1;
> >> +    buf_size--;
> >>  
> >>      region->width = AV_RB16(buf);
> >>      buf += 2;
> >> +    buf_size -= 2;
> >>      region->height = AV_RB16(buf);
> >>      buf += 2;
> >> +    buf_size -= 2;
> >
> >using buf_end is simpler and more robust than updating buf_size
> >synchronly with every buf update. buf_end is already there so this
> >seems unneeded
> >
> 
> 
> Michael, i have never liked using the convention of using a buffer end as being the correct way to process a structure, I prefer to process the buffer as the spec says as keep the counters decrementing as per the spec, and i have done a lot of work in the DVB field, as in my last 10 years ive been writing mainly settop box firmware.

well, everyone has their preferrance.
Id prefer

region->width = bytestream_get_be16(&buf);
region->height= bytestream_get_be16(&buf);

or

region->width = AV_RB16(buf+1)
region->height= AV_RB16(buf+3)

it looks alot simpler too me.

but this is really not important, its not the style thats causing me
a headache but the mixing of lots of unrelated changes, some of which
are style changes some of which are bugfixes.
It simply makes reviewing the change difficult, more time consuming
and more error prone



> 
> 
> Also a good compiler will join all these subtractions into a single subtract.
>
> Changes made to dvbsub*.c are to fix to the encoding and decoding, not real much to say but if you use the old source against the TS file i posted to the bug system months ago you will see that it did not work, it does now.

thank you, i will try to see if i can reproduce the issues and split
the related changes to dvbsub*.c out and commit them


-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110927/b09fa94a/attachment.asc>