[FFmpeg-devel] [PATCH 2/2] MxPEG decoder

[Anatoly Nenashev]

On 01.11.2010 15:55, Michael Niedermayer wrote:
> On Mon, Nov 01, 2010 at 11:52:54AM +0300, Anatoly Nenashev wrote:
>    
>> On 01.11.2010 03:31, Michael Niedermayer wrote:
>>      
>>> On Mon, Nov 01, 2010 at 02:56:50AM +0300, Anatoly Nenashev wrote:
>>>
>>>        
>>>> On 01.11.2010 01:17, Michael Niedermayer wrote:
>>>>
>>>>          
>>>>> On Sun, Oct 31, 2010 at 09:23:13PM +0300, Anatoly Nenashev wrote:
>>>>>
>>>>>
>>>>>            
>>>>>> Patch for .mxg demuxer
>>>>>>
>>>>>>
>>>>>>              
>>>>> [...]
>>>>>
>>>>>
>>>>>            
>>>>>> +static int mxg_read_header(AVFormatContext *s, AVFormatParameters *ap)
>>>>>> +{
>>>>>> +    AVStream *video_st = 0, *audio_st = 0;
>>>>>> +    MXGContext *mxg = s->priv_data;
>>>>>> +
>>>>>> +    /* video parameters will be extracted from the compressed bitstream */
>>>>>> +    video_st = av_new_stream(s, VIDEO_STREAM_INDEX);
>>>>>> +    if (!video_st)
>>>>>> +        return AVERROR(ENOMEM);
>>>>>> +    video_st->codec->codec_type = AVMEDIA_TYPE_VIDEO;
>>>>>> +    video_st->codec->codec_id = CODEC_ID_MXPEG;
>>>>>> +    av_set_pts_info(video_st, 64, 1, 1000000);
>>>>>> +
>>>>>> +    audio_st = av_new_stream(s, AUDIO_STREAM_INDEX);
>>>>>> +    if (!audio_st)
>>>>>> +        return AVERROR(ENOMEM);
>>>>>> +    audio_st->codec->codec_type = AVMEDIA_TYPE_AUDIO;
>>>>>> +    audio_st->codec->codec_id = CODEC_ID_PCM_ALAW;
>>>>>> +    audio_st->codec->channels = 1;
>>>>>> +    audio_st->codec->sample_rate = 8000;
>>>>>> +    audio_st->codec->bits_per_coded_sample = 8;
>>>>>> +    audio_st->codec->block_align = 1;
>>>>>> +
>>>>>>
>>>>>>
>>>>>>              
>>>>>
>>>>>            
>>>>>> +    mxg->buffer = (uint8_t*) av_malloc(DEFAULT_PACKET_SIZE);
>>>>>>
>>>>>>
>>>>>>              
>>>>> unneeded cast
>>>>>
>>>>>
>>>>>            
>>>> removed
>>>>
>>>>
>>>>          
>>>>> [...]
>>>>>
>>>>>
>>>>>            
>>>>>> +        if (mxg->found_video_packet) {
>>>>>> +            mxg->buffer[mxg->current_pos++] = data;
>>>>>> +            if (mxg->state == 0xfffe) {
>>>>>> +                int size = get_be16(s->pb);
>>>>>> +                if (url_feof(s->pb) || url_ferror(s->pb))
>>>>>> +                    return AVERROR_EOF;
>>>>>> +                mxg->buffer = av_fast_realloc(mxg->buffer,&mxg->buffer_size,
>>>>>> +                                              mxg->current_pos + size);
>>>>>>
>>>>>>
>>>>>>              
>>>>> here the + and similar at other points can overflow and lead to realloc to a
>>>>> too small buffer
>>>>>
>>>>>
>>>>>            
>>>> fixed
>>>>
>>>>
>>>>          
>>>>> [...]
>>>>>
>>>>>
>>>>>            
>>>>>> +            if (mxg->current_pos>= mxg->buffer_size) {
>>>>>> +                mxg->buffer = (uint8_t*) av_fast_realloc(mxg->buffer,&mxg->buffer_size,
>>>>>>
>>>>>>
>>>>>>              
>>>>> unneeded cast
>>>>>
>>>>>
>>>>>            
>>>> removed
>>>>
>>>>
>>>>          
>>> [...]
>>>
>>>        
>>>> +static int mxg_read_packet(AVFormatContext *s, AVPacket *pkt)
>>>> +{
>>>> +    int ret, size;
>>>> +    MXGContext *mxg = s->priv_data;
>>>> +
>>>> +    while(!url_feof(s->pb)&&   !url_ferror(s->pb)) {
>>>> +        uint8_t data;
>>>> +        int found_frame_end = 0;
>>>> +
>>>> +        if (mxg->state == 0xffed) {
>>>> +            size = get_be16(s->pb);
>>>> +            if (url_feof(s->pb) || url_ferror(s->pb))
>>>> +                return AVERROR_EOF;
>>>> +            if (size<= 14)
>>>> +                return AVERROR(EINVAL);
>>>> +            url_fskip(s->pb, 12);
>>>> +            if (url_feof(s->pb))
>>>> +                return AVERROR_EOF;
>>>> +            size -= 14;
>>>> +            ret = av_get_packet(s->pb, pkt, size);
>>>> +            if (ret<   0)
>>>> +                return ret;
>>>> +
>>>> +            pkt->stream_index = AUDIO_STREAM_INDEX;
>>>> +            mxg->state = 0;
>>>> +
>>>> +            return pkt->size;
>>>> +        }
>>>> +
>>>> +        data = get_byte(s->pb);
>>>> +        mxg->state = (mxg->state<<   8) | data;
>>>> +
>>>> +        if (mxg->found_video_packet) {
>>>> +            mxg->buffer[mxg->current_pos++] = data;
>>>> +            if (mxg->state == 0xfffe) {
>>>> +                int size = get_be16(s->pb);
>>>> +                if (url_feof(s->pb) || url_ferror(s->pb))
>>>> +                    return AVERROR_EOF;
>>>> +
>>>> +                if (mxg->current_pos>   mxg->current_pos + size)
>>>> +                {
>>>> +                    av_log(s, AV_LOG_ERROR, "buffer overflow\n");
>>>> +                    return AVERROR(ENOMEM);
>>>> +                }
>>>> +                mxg->buffer = av_fast_realloc(mxg->buffer,&mxg->buffer_size,
>>>> +                                              mxg->current_pos + size);
>>>> +                if (!mxg->buffer) {
>>>> +                    av_log(s, AV_LOG_ERROR, "mxg demuxer error in av_fast_realloc()\n");
>>>> +                    return AVERROR(ENOMEM);
>>>> +                }
>>>> +                if (size<   2) {
>>>> +                    av_log(s, AV_LOG_ERROR, "wrong comment buffer size\n");
>>>> +                    return AVERROR(EINVAL);
>>>> +                }
>>>> +                mxg->buffer[mxg->current_pos++] = size>>   8;
>>>> +                mxg->buffer[mxg->current_pos++] = size&   0xff;
>>>> +
>>>> +                ret = get_buffer(s->pb, mxg->buffer + mxg->current_pos, size - 2);
>>>> +                if (ret<   0)
>>>> +                    return ret;
>>>> +                if (ret>= 16&&   !strncmp(mxg->buffer + mxg->current_pos, "MXF", 3)) {
>>>> +                    mxg->dts = AV_RL64(mxg->buffer + mxg->current_pos + 8);
>>>> +                }
>>>> +                mxg->current_pos += ret;
>>>> +                mxg->state = 0;
>>>> +            } else if (mxg->state == 0xffd9) {
>>>> +                found_frame_end = mxg->current_pos;
>>>> +                mxg->current_pos = 0;
>>>> +                mxg->found_video_packet = 0;
>>>> +            } else if (mxg->state == 0xffd8) {
>>>> +                //emulating frame end
>>>> +                found_frame_end = mxg->current_pos - 2;
>>>> +                mxg->current_pos = 2;
>>>> +                mxg->found_video_packet = 1;
>>>> +            } else if (mxg->state == 0xffed) {
>>>> +                //emulating frame end
>>>> +                found_frame_end = mxg->current_pos - 2;
>>>> +                mxg->current_pos = 0;
>>>> +                mxg->found_video_packet = 0;
>>>> +            }
>>>> +
>>>> +            if (found_frame_end) {
>>>> +                ret = av_new_packet(pkt, found_frame_end);
>>>> +                if (ret<   0)
>>>> +                    return ret;
>>>> +                memcpy(pkt->data, mxg->buffer, found_frame_end);
>>>> +                pkt->stream_index = VIDEO_STREAM_INDEX;
>>>> +                pkt->dts = mxg->dts;
>>>> +                return pkt->size;
>>>> +            }
>>>> +
>>>> +            if (mxg->current_pos>= mxg->buffer_size) {
>>>> +                if (mxg->current_pos>   mxg->current_pos + DEFAULT_PACKET_SIZE)
>>>> +                {
>>>> +                    av_log(s, AV_LOG_ERROR, "buffer overflow\n");
>>>> +                    return AVERROR(ENOMEM);
>>>> +                }
>>>> +                mxg->buffer = av_fast_realloc(mxg->buffer,&mxg->buffer_size,
>>>> +                                              mxg->current_pos + DEFAULT_PACKET_SIZE);
>>>> +                if (!mxg->buffer) {
>>>> +                    av_log(s, AV_LOG_ERROR, "mxg demuxer error in av_fast_realloc()\n");
>>>> +                    return AVERROR(ENOMEM);
>>>> +                }
>>>> +            }
>>>> +        } else if (mxg->state == 0xffd8){
>>>> +            mxg->found_video_packet = 1;
>>>> +        }
>>>>
>>>>          
>>> This loop is a bit oddly structured, is there a reason for this?
>>> id expect it to be more of the form
>>> while {
>>> state= (state<<8) + ...
>>>
>>> if(state ==)
>>> else if
>>> else if
>>> else
>>> }
>>>
>>> but its kind of convoluted with the audio code being before the state update
>>> then found_video_packet=1 and checks for that
>>>
>>>
>>>        
>> Reimplemented. I hope that new version is more canonical.:-)
>>      
> no its the same
> id like to understand why it is so convolutedly written if there is a reason
> why this is needed
> If its really needed this has to be documented so someone doesnt waste time
> rewriting it to half the code.
> And if there is no reason it should be simplified
>    
>

The principal reason consists in the following. If demuxer yet hasn't 
found a starting marker for package video it should check much less 
conditions, than in other case. Moreover in the second case it should 
copy data from incoming stream into the internal buffer because we don't 
know the size of video packet. That's why I have divided these two cases 
in a program code in hope that so will be faster.