[Ffmpeg-devel] [RFC] av_seek_frame behaviour

[Baptiste Coudurier]

Reimar D?ffinger a ?crit :
>>>>> [...]
 >>>>>
>>>>> If anything, introduce two different return values, one indicating that
>>>>> the frame definitely is not there and another one to indicate that we
>>>>> should try the other seek methods.
>>>>> But that still will not fix your problem. From the description I'd say
>>>>> what you want is a FAST_SEEK_ONLY flag or similar that would only allow
>>>>> seek methods with O(log n) max.
>> Basically, no. I'd like the seek to just fail and not trying anything 
>> else, since there is absolutely no packet with that timestamp if MXF 
>> seek function fails.
> 
> Even with other formats, that is assuming the file is not broken...

Let's fix working files before thinking about broken files.

> In the case of MXF, this seems to be a simple bit-rate based seek and in
> addition there is also
>> if (!s->bit_rate)
>>   return -1;
> 
> So either it can fail even in cases where the time exists or that should
> be an assert ;-)
> There also is a "/* rudimentary binary seek */" comment above
> mxf_read_seek, but I can't see any binary search being done? Or what
> should that comment tell me?

It is a global bitrate based seeking, MXF contains no timestamp per 
packet, if bitrate is not present then no way to extrapolate position 
based on file size. Its not correct, but it somehow works, and it is 
fast, that's why it is rudimentary, now I'd be happy with a seeking 
function which uses index, but there aren't many around.

Comment might not be wise, yes, it's more "rudimentary byte seek".

>>>>> (which is another things that IMO is
>>>>> wrong with that patch, av_seek_frame_binary is O(log n), so except for
>>>>> streamed content there is not much reason to try that as well.
>>>> That has nothing to do with my patch, current code will always try 
>>>> av_seek_frame_binary if demuxer has read_timestamp and no seek function 
>>>> or if its seek function returned < 0.
>>> ?? Why do my suggestions to fix the problem in a "proper" way (i.e. without
>>> removing the feature of falling back to possibly more reliable seek methods
>>> as your patch does) have nothing to do with your patch?
>> What's you suggestion ? "av_seek_frame_binary is O(log n), so except for 
>> stream context there is not much reason to try that as well ?"
> 
> I was only pondering if a SEEK_FLAG_NOBINARY would even make sense...

Well there is AVSEEK_BYTE flag, so AVSEEK_NO_BINARY, but that will 
change API.
Also I don't have problems with binary, I have a problem with
av_seek_frame_generic.

>>>> What about calling av_seek_frame_generic only for formats having 
>>>> AVFMT_GENERIC_INDEX ?
>>> Well, IMO any such fixed problem does not solve what I feel is the real
>>> problem: Some people will want fast seeks, others need them to be as
>>> precise as possible.
>> Real problem is to fail quickly when it has to fail. Trying other 
>> methods is a plus. So maybe returning AVSEEK_FAIL, AVSEEK_TRY_BINARY, 
>> AVSEEK_TRY_GENERIC, or AVSEEK_FLAG_TRY_ALL user can pass.
>>
>> A failing demuxer seek function should not mean that other methods 
>> should be tried.
> 
> Depends on the meaning of "fail". It could "fail" because that position
> absolutely certainly isn't there. Or it could "fail" because it tries to
> lazily read the index and finds out it can't e.g. because it's
> corrupted.

Well, define "fail" and check against return values.

> Of course the fallback to other methods could be done in each demuxer,
> but using return values to indicate how the seek failed seems more
> robust and simpler to me in the long term.

Well if you vonlonteer to check all demuxers, Im ok, else I would more 
in favor or fixing GXF seek so It always uses av_seek_frame_binary if 
possible of course.

>> av_seek_frame_generic when no index is present does read all packets 
>> since the beginning that is clearly too expansive and should not be done 
>> unless explicitely asked.
> 
> I agree it should be possible to disable, but "clearly too expansive" (I
> assume you meant expensive) maybe be true in your case, but for
> lowres MPEG4-compressed movies stored on HD of around 800 MB (which I would
> consider more the normal case) it seems still acceptable to me.

Im talking about av_seek_frame_generic here. If you are talking about 
binary, I don't know.

Also which normal case ? In which container ?

I run a p3 1ghz here and av_seek_frame_generic is clearly not 
acceptable, only for small files mainly FLV (for which that 
av_seek_frame_generic behaviour changed). Now every seeking failure with 
a format that has no read_timestamp function will be parsed from the 
beginning.

>>> And what about av_seek_frame_binary?  till always calling it whenever
>>> the "main" seek function is fails?
>> IMO if demuxer knows that av_seek_frame_binary works, it should call it
>> itself from its seek function.
> 
> Or it could indicate that fact via the return value, which would allow
> to avoid code duplication and maintain backwards-compatibility, or does

If seek functions call functions they want to use when seeking it will 
maintain backwards-compatibility, now speaking about that compatibility 
av_seek_frame_generic broke it.

I feel that those return values could introduce side effects (like it 
did for MXF). Also you would have to modify all demuxers and check their 
return values, and their behaviour.

If GXF only needs that behaviour, and GXF seek can always uses 
av_seek_frame_binary , it will remove some code (gxf own seek function), 
and avoid that API modification.

I just noticed that TS does use av_seek_frame_binary, and returns -1, 
and then current code will redo av_seek_frame_binary since demuxer has 
read_timestamp function. Nice.

> someone volunteer to check all demuxers if they make assume that
> fallback like gxf does? ;-)

I hope you are.
I'd like to get things fixed, in nice ways, but I'd like having them 
fixed and not dismissed.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no