[Ffmpeg-devel] [PATCH] FLV decoder metadata reading
Allan Hsu
allan
Sun Dec 10 15:11:44 CET 2006
On Dec 10, 2006, at 1:48 AM, Michael Niedermayer wrote:
> Hi
>
> On Sat, Dec 09, 2006 at 07:25:11PM -0800, Allan Hsu wrote:
> [...]
>
>>>
>>>>
>>>> Fixed.
>>>>
>>>> [...]
>>>>>> + if( flv_read_tag_header(&s->pb, &taginfo) < 0 ||
>>>>>> taginfo.type != FLV_TAG_TYPE_META
>>>>>> + || flv_read_metabody(s, taginfo.next_pos) < 0 ||
>>>>>> flv_validate_context(s) < 0
>>>>>> + || flv_create_streams(s) < 0) {
>>>>>> + //error reading first tag header, first tag is not
>>>>>> metadata, or metadata incomplete.
>>>>>> + s->ctx_flags |= AVFMTCTX_NOHEADER;
>>>>>> +
>>>>>
>>>>> isnt it simpler to simple read all the metadata in flv_read_packet
>>>>> () and just
>>>>> call flv_read_packet() from flv_read_header() once if needed?
>>>>>
>>>>>> if(!url_is_streamed(&s->pb)){
>>>>>> const int fsize= url_fsize(&s->pb);
>>>>>> url_fseek(&s->pb, fsize-4, SEEK_SET);
>>>>>> @@ -62,7 +387,8 @@
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> - url_fseek(&s->pb, offset, SEEK_SET);
>>>>>> + url_fseek(&s->pb, offset, SEEK_SET);
>>>>>
>>>>> this seeks backward or? if so its a matter of luck if it works,
>>>>> if theres
>>>>> too much metadata then it will fail if the stream is not
>>>>> "seekable"
>>>> [...]
>>>>
>>>> I tried to implement your suggested changes to flv_read_header()
>>>> and
>>>> flv_read_packet(), but it only made the backward-seeking issue
>>>> worse.
>>>> Since metadata packets don't have a stream, they don't have a valid
>>>> stream index and so the next audio/video packet is returned by
>>>> flv_read_packet(), which makes the maximum length of a backward
>>>> seek
>>>> in flv_read_header() quite large.
>>>>
>>>> Instead, I've opted to try and reduce the size of any backward
>>>> seeks in
>>>> the attached revision of the patch. Backward seeks will only
>>>> occur in
>>>> the case that there is not enough metadata. In this case, if the
>>>> first
>>>> tag was a metadata tag, the backwards seek should go to the second
>>>> packet, where flv_read_metabody() should have stopped, which should
>>>> be effectively no movement. If the first tag wasn't a metadata tag,
>>>> the backward seek should be 15 bytes (the size of the FLV tag
>>>> header
>>>> read by flv_read_tag_header()).
>>>
>>> what about putting the inside of the for() loop from
>>> flv_read_packet()
>>> into its own function and then calling that from flv_read_packet
>>> () and
>>> flv_read_header()
>>> that way FLVTagInfo and flv_read_tag_header() should become unneeded
>>
>> I've looked at this and I'm not sure if this would help all that
>> much. There is a lot of logic inside the for() loop that wants to
>> skip the current packet and try the next packet before exiting the
>> loop. To keep that logic around, we would either have to leave it in
>> the loop or push the looping behavior into the separate function.
>>
>> I tried a version that left the tag skipping logic in the loop, and
>> the separate function ended up being almost identical to
>> flv_read_tag_header() except with a special case for metadata tag
>> handling. This function didn't make a lot of sense, functionality-
>> wise, and it ended up passing a large amount of data back, which
>> means FLVTagInfo never really went away. flv_read_packet() still
>> requires this information after calling this function: body size, tag
>> type, next_pos, and pts.
>>
>> I didn't try the version that pushed the looping into the separate
>> function because it seemed like this function would have caused the
>> same large backwards-seek issue we were trying to avoid in
>> flv_read_header() or it would have an odd set of responsibilities.
>> In the metadata reading case, the function would:
>>
>> seek backwards if first packet is not metadata
>> parse metadata otherwise, seek to second packet before returning.
>> create streams if metadata is complete and valid.
>>
>> In the packet reading case:
>>
>> skip past any non-audio/video packets
>> skip past any st->discard designated packets
>> return information required by the rest of flv_read_packet(): body
>> size, tag type, next tag position, tag pts, tag flags
>>
>> I think the problem here is that the largest amount of shared
>> function between the two usages is that both flv_read_header() and
>> flv_read_packet() need to know a minimal amount of data about the
>> next tag in the stream before deciding what to do with them. This is
>> what flv_read_tag_header() does. After this, flv_read_header() either
>> processes the tag or seeks back to the beginning the tag.
>> flv_read_packet(), however, skips tags until it finds one that's
>> interesting. I'm at a loss as to how we could encapsulate these two
>> kinds of operations into one sane-looking function.
>
> hmm, what about leaving the metadata reading in flv_read_packet() and
> clearing AVFMTCTX_NOHEADER as soon as all needed data is available?
This would cause flv_read_header() to seek to the end of the stream
if !url_is_streamed() when it tries to determine duration, which
undesirable in my case, where I want to begin decoding of a partially-
downloaded FLV before the entire file is available. In this case, the
FLV isn't "streamed" in the non-seekable sense, but a seek and read
from the end would block until that portion of the file is available.
>>
>> [...]
>>>> +static int flv_validate_context(AVFormatContext *s) {
>>>> + FLVDemuxContext *context = s->priv_data;
>>>> +
>>>> + //if any values do not validate, assume metadata tool was
>>>> brain dead and fail.
>>>> + if(s->duration <= 0)
>>>> + return -1;
>>>> +
>>>> + if(context->has_audio) {
>>>> + switch(context->audiocodecid <<
>>>> FLV_AUDIO_CODECID_OFFSET) {
>>>> + case FLV_CODECID_PCM_BE:
>>>> + case FLV_CODECID_ADPCM:
>>>> + case FLV_CODECID_MP3:
>>>> + case FLV_CODECID_PCM_LE:
>>>> + case FLV_CODECID_NELLYMOSER_8HZ_MONO:
>>>> + case FLV_CODECID_NELLYMOSER:
>>>> + break;
>>>> + default:
>>>> + return -1;
>>>
>>> whats invalid if the codec id is not one of these?
>>> same for the samplerate and video?
>>
>> I'm not sure what you're asking here? If the values in the metadata
>> tag aren't valid values for what they describe, either the metadata
>> tag is supplying bogus data (and we should ignore it and hope we can
>> read the stream the old-fashioned way) or some new capabilities have
>> been added to the FLV spec and our decoder needs to be updated:).
>> I've added some logging lines on the case of failed validation.
>
> well my reasoning is that a unknown value in the codec tag does not
> render
> all other values invalid, the samplerate or width and height still
> likely
> are correct and ffmpeg -vcodec copy should be able to just copy such a
> stream between 2 flv files, for that of course the width/height/
> samplerate
> must be available otherwise at least the metadata would dissapear ...
flv_validate_context() only functions as a check to see if the
information read from the FLV header and metadata tag is complete
enough to create the streams. I've changed the name of
flv_validate_context() to flv_check_meta_values() to make this more
clear.
I think what you're describing is the responsibility of the stream
creation code in flv_read_packet(). It already contains code like this:
default:
av_log(s, AV_LOG_INFO, "Unsupported video codec (%x)\n", flags &
FLV_VIDEO_CODECID_MASK);
st->codec->codec_tag = flags & FLV_VIDEO_CODECID_MASK;
I think it could be changed to something like this:
default: {
FLVDemuxContext *context = s->priv_data;
av_log(s, AV_LOG_INFO, "Unsupported video codec (%x)\n", flags &
FLV_VIDEO_CODECID_MASK);
st->codec->codec_tag = flags & FLV_VIDEO_CODECID_MASK;
if(context->height > 0) st->codec->height = context->height;
if(context->width > 0) st->codec->width = context->width;
}
This should give the muxer enough information to recreate the video
metadata that we read from the metadata tag. Something similar could
be done for unknown audio codecs as well.
IMHO, this functionality should be the subject of a different series
of patches that also address these issues:
1) the FLV muxer does not currently write this metadata.
2) the FLV muxer is currently broken such that -vcodec copy produces
bad output for non-H263 video codecs (video tags are unconditionally
marked as H263).
[...]
> this thing is missing the AMF_DATA_TYPE_MIXEDARRAY i think?
Added.
[...]
>> + url_fseek(ioc, next_pos, SEEK_SET);
>> + return 0;
>> +
>> +bail:
>> + url_fseek(ioc, next_pos, SEEK_SET);
>> + return -1;
>> +}
>
> putting url_fseek(ioc, next_pos, SEEK_SET); after both calls to
> flv_read_metabody() would allow you to replace the goto bail by
> return -1
[...]
Changed.
New patch attached.
-Allan
--
Allan Hsu <allan at counterpop dot net>
1E64 E20F 34D9 CBA7 1300 1457 AC37 CBBB 0E92 C779
-------------- next part --------------
A non-text attachment was scrubbed...
Name: flvdec_metadata.patch
Type: application/octet-stream
Size: 17347 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20061210/de835f5e/attachment.obj>
-------------- next part --------------
More information about the ffmpeg-devel
mailing list