[FFmpeg-devel] [PATCH] H.264/AVCHD interlaced fixes

[Michael Niedermayer]

On Thu, Feb 19, 2009 at 05:43:14PM +0100, Ivan Schreter wrote:
> Michael Niedermayer wrote:
>> On Thu, Feb 19, 2009 at 12:51:52PM +0100, Ivan Schreter wrote:
>>   [...]
>>
>>   
>>> @@ -1385,11 +1386,11 @@
>>>  # if HAVE_FAST_64BIT
>>>  #   define RS 7
>>>      for(i=0; i+1<length; i+=9){
>>> -        if(!((~*(uint64_t*)(src+i) & (*(uint64_t*)(src+i) - 
>>> 0x0100010001000101ULL)) & 0x8000800080008080ULL))
>>> +        if(!((~*(const uint64_t*)(src+i) & (*(const uint64_t*)(src+i) - 
>>> 0x0100010001000101ULL)) & 0x8000800080008080ULL))
>>>  # else
>>>  #   define RS 3
>>>      for(i=0; i+1<length; i+=5){
>>> -        if(!((~*(uint32_t*)(src+i) & (*(uint32_t*)(src+i) - 
>>> 0x01000101U)) & 0x80008080U))
>>> +        if(!((~*(const uint32_t*)(src+i) & (*(const uint32_t*)(src+i) - 
>>> 0x01000101U)) & 0x80008080U))
>>>  # endif
>>>              continue;
>>>          if(i>0 && !src[i]) i--;
>>>     
>>
>> this does not belong in this patch
>>
>>   
> OK, separate patch for const warning attached.
>
>>> @@ -1411,9 +1412,9 @@
>>>      }
>>>       if(i>=length-1){ //no escaped 0
>>> -        *dst_length= length;
>>>          *consumed= length+1; //+1 for the header
>>> -        return src;
>>> +        ret= src;
>>> +        goto find_bitlength;
>>>      }
>>>       bufidx = h->nal_unit_type == NAL_DPC ? 1 : 0; // use second escape 
>>> buffer for inter data
>>> @@ -1450,27 +1451,34 @@
>>>       memset(dst+di, 0, FF_INPUT_BUFFER_PADDING_SIZE);
>>>  -    *dst_length= di;
>>> +    length= di;
>>>      *consumed= si + 1;//+1 for the header
>>>     
>>
>>   
>>> -//FIXME store exact number of bits in the getbitcontext (it is needed 
>>> for decoding)
>>> -    return dst;
>>> -}
>>> +    ret= dst;
>>>  -/**
>>> - * identifies the exact end of the bitstream
>>> - * @return the length of the trailing, or 0 if damaged
>>> - */
>>> -static int decode_rbsp_trailing(H264Context *h, const uint8_t *src){
>>> -    int v= *src;
>>> -    int r;
>>>     
>>
>> iam not happy that these 2 functions are merged
>>
>>   
> Another patch attached, which preserves the original function, although 
> used only at single point.
>
> I had to move it, since it's used from decode_nal, was define after it, 
> though.
>
>> with factorization i did not mean to merge functions in a messy large blob
>> but to rather create a NEW function that factorizes the common code from
>> the 2 instances
>>   
> Well, there is the code to find exact bit size, which is in h264.c and is 
> also needed for the parser. So instead of duplicating this code, I moved 
> the part (a few lines) from decode_nal_units() to decode_nal(). I also 
> merged decode_rbsp_trailing(), since it's not used anywhere else. I've 
> unmerged this one now.
>
> Another point is creating NEW function for decode_slice_header() to be used 
> from h264 and h264_parser, but that one will be trickier.
>
> I hope the attached new version of the patch for decode_nal() is now OK.
>
> Regards,
>
> Ivan
>

> Index: libavcodec/h264.c
> ===================================================================
> --- libavcodec/h264.c	(revision 17452)
> +++ libavcodec/h264.c	(working copy)
> @@ -1385,11 +1386,11 @@
>  # if HAVE_FAST_64BIT
>  #   define RS 7
>      for(i=0; i+1<length; i+=9){
> -        if(!((~*(uint64_t*)(src+i) & (*(uint64_t*)(src+i) - 0x0100010001000101ULL)) & 0x8000800080008080ULL))
> +        if(!((~*(const uint64_t*)(src+i) & (*(const uint64_t*)(src+i) - 0x0100010001000101ULL)) & 0x8000800080008080ULL))
>  # else
>  #   define RS 3
>      for(i=0; i+1<length; i+=5){
> -        if(!((~*(uint32_t*)(src+i) & (*(uint32_t*)(src+i) - 0x01000101U)) & 0x80008080U))
> +        if(!((~*(const uint32_t*)(src+i) & (*(const uint32_t*)(src+i) - 0x01000101U)) & 0x80008080U))
>  # endif
>              continue;
>          if(i>0 && !src[i]) i--;

hunk/patch ok


> Index: libavcodec/h264.c
> ===================================================================
> --- libavcodec/h264.c	(revision 17452)
> +++ libavcodec/h264.c	(working copy)
> @@ -1360,15 +1360,33 @@
>  }
> 
>  /**
> + * identifies the exact end of the bitstream
> + * @return the length of the trailing, or 0 if damaged
> + */
> +static int decode_rbsp_trailing(H264Context *h, const uint8_t *src){
> +    int v= *src;
> +    int r;
> +
> +    tprintf(h->s.avctx, "rbsp trailing %X\n", v);
> +
> +    for(r=1; r<9; r++){
> +        if(v&1) return r;
> +        v>>=1;
> +    }
> +    return 0;
> +}

[...]

> -/**
> - * identifies the exact end of the bitstream
> - * @return the length of the trailing, or 0 if damaged
> - */
> -static int decode_rbsp_trailing(H264Context *h, const uint8_t *src){
> -    int v= *src;
> -    int r;
> -
> -    tprintf(h->s.avctx, "rbsp trailing %X\n", v);
> -
> -    for(r=1; r<9; r++){
> -        if(v&1) return r;
> -        v>>=1;

moving code must be in a seperate patch, or actually cosmetics and functional
changes should be in seperate patches.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090219/7984192f/attachment.pgp>