Re: [FFmpeg-devel] [PATCH v2] avformat/ifv: added support for ifv cctv files
On 5/4/19, Swaraj Hota wrote: > Fixes ticket #2956. > > Signed-off-by: Swaraj Hota > --- > Revised patch. Made some minor changes based on original player: > - Removed incorrect reading of frame_rate, instead frame rate > is kept fixed at 25 (seems like this value is always same). > - Added reading of frame width and height from input file. > Looks good, I gonna apply it in next few days. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avformat/ifv: added support for ifv cctv files
Okay. Thanks! ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avformat/ifv: added support for ifv cctv files
On 5/4/19, Swaraj Hota wrote: > Okay. Thanks! > Sorry, but I found some issues while testing your patch. Why are there no timestamps? Seeking with mpv/ffplay caused hard crash. You do not handle EOF, so ffmpeg with .ivf never exits. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avformat/ifv: added support for ifv cctv files
Some more or less important issues found: On 5/4/19, Swaraj Hota wrote: > Fixes ticket #2956. > > Signed-off-by: Swaraj Hota > --- > Revised patch. Made some minor changes based on original player: > - Removed incorrect reading of frame_rate, instead frame rate > is kept fixed at 25 (seems like this value is always same). > - Added reading of frame width and height from input file. > > --- > Changelog| 1 + > libavformat/Makefile | 1 + > libavformat/allformats.c | 1 + > libavformat/ifv.c| 335 +++ > libavformat/version.h| 4 +- > 5 files changed, 340 insertions(+), 2 deletions(-) > create mode 100644 libavformat/ifv.c > > diff --git a/Changelog b/Changelog > index a3fa0c14a2..03d2a9db01 100644 > --- a/Changelog > +++ b/Changelog > @@ -26,6 +26,7 @@ version : > - lscr decoder > - lagfun filter > - asoftclip filter > +- IFV demuxer > > > version 4.1: > diff --git a/libavformat/Makefile b/libavformat/Makefile > index 99be60d184..f68d41e4a5 100644 > --- a/libavformat/Makefile > +++ b/libavformat/Makefile > @@ -231,6 +231,7 @@ OBJS-$(CONFIG_ICO_MUXER) += icoenc.o > OBJS-$(CONFIG_IDCIN_DEMUXER) += idcin.o > OBJS-$(CONFIG_IDF_DEMUXER) += bintext.o sauce.o > OBJS-$(CONFIG_IFF_DEMUXER) += iff.o > +OBJS-$(CONFIG_IFV_DEMUXER) += ifv.o > OBJS-$(CONFIG_ILBC_DEMUXER) += ilbc.o > OBJS-$(CONFIG_ILBC_MUXER)+= ilbc.o > OBJS-$(CONFIG_IMAGE2_DEMUXER)+= img2dec.o img2.o > diff --git a/libavformat/allformats.c b/libavformat/allformats.c > index d316a0529a..cd00834807 100644 > --- a/libavformat/allformats.c > +++ b/libavformat/allformats.c > @@ -188,6 +188,7 @@ extern AVOutputFormat ff_ico_muxer; > extern AVInputFormat ff_idcin_demuxer; > extern AVInputFormat ff_idf_demuxer; > extern AVInputFormat ff_iff_demuxer; > +extern AVInputFormat ff_ifv_demuxer; > extern AVInputFormat ff_ilbc_demuxer; > extern AVOutputFormat ff_ilbc_muxer; > extern AVInputFormat ff_image2_demuxer; > diff --git a/libavformat/ifv.c b/libavformat/ifv.c > new file mode 100644 > index 00..f2d12b4fe7 > --- /dev/null > +++ b/libavformat/ifv.c > @@ -0,0 +1,335 @@ > +/* > + * IFV demuxer > + * > + * Copyright (c) 2019 Swaraj Hota > + * > + * This file is part of FFmpeg. > + * > + * FFmpeg is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * FFmpeg is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with FFmpeg; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 > USA > + */ > + > +#include "avformat.h" > +#include "internal.h" > +#include "avio_internal.h" > + > +#define IFV_MAGIC "\x11\xd2\xd3\xab\xba\xa9\xcf\x11\ > +\x8e\xe6\x00\xc0\x0c\x20\x53\x65\x44" > + > +typedef struct IFVIndexEntry { > +uint32_t frame_offset; > +uint32_t frame_size; > +enum AVMediaType frame_type; > +} IFVIndexEntry; > + > +typedef struct IFVContext { > +IFVIndexEntry *vid_index; > +IFVIndexEntry *aud_index; > +uint32_t index_pos; > +uint32_t total_vframes; > +uint32_t total_aframes; > + > +int width, height; > +int is_audio_present; > +int sample_rate; > + > +int video_stream_index; > +int audio_stream_index; > +} IFVContext; > + > +static int ifv_probe(const AVProbeData *p) > +{ > +if (!memcmp(p->buf, IFV_MAGIC, 17)) > +return AVPROBE_SCORE_MAX; > +return 0; > +} > + > +static int read_next_index_entry(AVFormatContext *s, enum AVMediaType > frame_type) > +{ > +IFVContext *ifv = s->priv_data; > +IFVIndexEntry *e; > + > +if (frame_type == AVMEDIA_TYPE_VIDEO) { > +if (ifv->index_pos >= ifv->total_vframes) > +return 0; > + > +e = &ifv->vid_index[ifv->index_pos++]; > + > +e->frame_offset = avio_rl32(s->pb); > +e->frame_size = avio_rl32(s->pb); > +e->frame_type = frame_type; > + > +avio_skip(s->pb, 20); Are PTS stored here? > + > +} else if (frame_type == AVMEDIA_TYPE_AUDIO) { > +if (ifv->index_pos >= ifv->total_aframes) > +return 0; > + > +e = &ifv->aud_index[ifv->index_pos++]; > + > +e->frame_offset = avio_rl32(s->pb); > +e->frame_size = avio_rl32(s->pb); > +e->frame_type = frame_type; > + > +avio_skip(s->pb, 16); Are PTS stored here? > +} > + > +return 1; > +} > + > +static int read_index(AVFormatCont
Re: [FFmpeg-devel] [PATCH v2] avformat/ifv: added support for ifv cctv files
Hello! Nothing major, but a few comments on things that might make sense to polish below. On Sat, May 04, 2019 at 06:42:40PM +0530, Swaraj Hota wrote: > +#define IFV_MAGIC "\x11\xd2\xd3\xab\xba\xa9\xcf\x11\ > +\x8e\xe6\x00\xc0\x0c\x20\x53\x65\x44" > +if (!memcmp(p->buf, IFV_MAGIC, 17)) Using an array and sizeof() instead of the hard-coded 17 might be a bit nicer. > +int ret; > + > +if (frame_type == AVMEDIA_TYPE_VIDEO) { > +ret = av_reallocp_array(&ifv->vid_index, ifv->total_vframes, > sizeof(IFVIndexEntry)); > +if (ret < 0) > +return ret; > + > +} else if (frame_type == AVMEDIA_TYPE_AUDIO) { > +ret = av_reallocp_array(&ifv->aud_index, ifv->total_aframes, > sizeof(IFVIndexEntry)); > +if (ret < 0) > +return ret; > +} Could just declare "ret" right where it is assigned. > +avio_skip(s->pb, 0x5c); If for any of these skips you have any idea what data they contain, it would be nice to document it. > +if (aud_magic == MKTAG('G','R','A','W')) { > +ifv->is_audio_present = 1; > +} else if (aud_magic == MKTAG('P','C','M','U')) { > +ifv->is_audio_present = 0; > +} else { > +avpriv_request_sample(s, "Unknown audio codec %x\n", aud_magic); > +} Why does PCMU mean "no audio"? Could you add a comment? Also, wouldn't it be more consistent to explicitly set is_audio_present in the "else" case? > +/*read video index*/ > +avio_seek(s->pb, 0xf8, SEEK_SET); [...] > +avio_skip(s->pb, ifv->vid_index->frame_offset - avio_tell(s->pb)); Why use avio_seek in one place and avio_skip in the other? > +pos = avio_tell(s->pb); > + > +for (i = 0; i < ifv->total_vframes; i++) { > +e = &ifv->vid_index[i]; > +if (e->frame_offset >= pos) > +break; > +} This looks rather inefficient. Wouldn't it make more sense to either use a binary search or at least to remember the position from the last read? This also does not seem very robust either, if a single frame_offset gets corrupted to a very large value, this code will never be able to find the "correct" position. It seems to assume the frame_offset is ordered increasingly (as would be needed for binary search), but that property isn't really checked/enforced. > +av_freep(&ifv->vid_index); > +if (ifv->is_audio_present) > +av_freep(&ifv->aud_index); Shouldn't the if () be unnecessary? Best regards, Reimar Döffinger ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avformat/ifv: added support for ifv cctv files
Am So., 5. Mai 2019 um 21:59 Uhr schrieb Reimar Döffinger : > > +avio_skip(s->pb, 0x5c); > > If for any of these skips you have any idea what data they > contain, it would be nice to document it. Iirc, all values we understood are used now. > > +if (aud_magic == MKTAG('G','R','A','W')) { > > +ifv->is_audio_present = 1; > > +} else if (aud_magic == MKTAG('P','C','M','U')) { > > +ifv->is_audio_present = 0; > > +} else { > > +avpriv_request_sample(s, "Unknown audio codec %x\n", aud_magic); > > +} > > Why does PCMU mean "no audio"? We have several files with "PCMU" and several files with "GRAW". All files with GRAW contain audio data, none of the files with PCMU contain audio data. I originally thought that the two bytes at 0x40/0x41 (iirc) show number of streams but a test with the proprietary player confirmed that this is not correct. Another test would be to overwrite GRAW with PCMU to find out if audio is still played... Thank you for your comments, Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avformat/ifv: added support for ifv cctv files
On Sun, May 05, 2019 at 10:05:17PM +0200, Carl Eugen Hoyos wrote: > Am So., 5. Mai 2019 um 21:59 Uhr schrieb Reimar Döffinger > : > > > +if (aud_magic == MKTAG('G','R','A','W')) { > > > +ifv->is_audio_present = 1; > > > +} else if (aud_magic == MKTAG('P','C','M','U')) { > > > +ifv->is_audio_present = 0; > > > +} else { > > > +avpriv_request_sample(s, "Unknown audio codec %x\n", aud_magic); > > > +} > > > > Why does PCMU mean "no audio"? > > We have several files with "PCMU" and several files with "GRAW". > All files with GRAW contain audio data, none of the files with PCMU > contain audio data. > I originally thought that the two bytes at 0x40/0x41 (iirc) show number > of streams but a test with the proprietary player confirmed that this > is not correct. > Another test would be to overwrite GRAW with PCMU to find out if > audio is still played... Ok, makes sense. I think it's good (and good enough) to have a comment that this is just heuristics/guesswork based on existing samples and might be quite wrong. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avformat/ifv: added support for ifv cctv files
On Sun, May 05, 2019 at 09:59:01PM +0200, Reimar Döffinger wrote: > Hello! > Nothing major, but a few comments on things that might make > sense to polish below. > > On Sat, May 04, 2019 at 06:42:40PM +0530, Swaraj Hota wrote: > > +#define IFV_MAGIC "\x11\xd2\xd3\xab\xba\xa9\xcf\x11\ > > +\x8e\xe6\x00\xc0\x0c\x20\x53\x65\x44" > > > +if (!memcmp(p->buf, IFV_MAGIC, 17)) > > Using an array and sizeof() instead of the hard-coded 17 might be a bit > nicer. > That will be another way to do it. I'll change it then. > > +int ret; > > + > > +if (frame_type == AVMEDIA_TYPE_VIDEO) { > > +ret = av_reallocp_array(&ifv->vid_index, ifv->total_vframes, > > sizeof(IFVIndexEntry)); > > +if (ret < 0) > > +return ret; > > + > > +} else if (frame_type == AVMEDIA_TYPE_AUDIO) { > > +ret = av_reallocp_array(&ifv->aud_index, ifv->total_aframes, > > sizeof(IFVIndexEntry)); > > +if (ret < 0) > > +return ret; > > +} > > Could just declare "ret" right where it is assigned. > Okay that could be done. > > > +/*read video index*/ > > +avio_seek(s->pb, 0xf8, SEEK_SET); > [...] > > +avio_skip(s->pb, ifv->vid_index->frame_offset - avio_tell(s->pb)); > > Why use avio_seek in one place and avio_skip in the other? > No particular reason. Essentially all are just skips. There is no backward seek. I left two seeks becuase they seemed more readable. Someone could know at a glance at what offset the first video and audio index are assumed/found to be. Should I change them to skips as well? > > +pos = avio_tell(s->pb); > > + > > +for (i = 0; i < ifv->total_vframes; i++) { > > +e = &ifv->vid_index[i]; > > +if (e->frame_offset >= pos) > > +break; > > +} > > This looks rather inefficient. > Wouldn't it make more sense to either > use a binary search or at least to > remember the position from the last read? > This also does not seem very robust either, > if a single frame_offset gets corrupted > to a very large value, this code will > never be able to find the "correct" position. > It seems to assume the frame_offset > is ordered increasingly (as would be needed for > binary search), but that property isn't > really checked/enforced. > Yeah it is indeed inefficient. But it also seems like the "correct" one. Because in case of seeking we might not be at the boundary of a frame and hence might need to skip to the boundary of next frame we can find. I guess this rules out binary search, and maybe also saving the last read. Regarding the frame_offset corruption, well that rules out binary search as well because then the order of the index will be disturbed. Or maybe I misunderstood? Please do mention if this can be done more efficiently by some method. I really need some ideas on this if it can be done. > > +av_freep(&ifv->vid_index); > > +if (ifv->is_audio_present) > > +av_freep(&ifv->aud_index); > > Shouldn't the if () be unnecessary? > Yeah I guess it is unnecessary. I'll change it. Thanks for the review! ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avformat/ifv: added support for ifv cctv files
On 07.05.2019, at 12:00, Swaraj Hota wrote: > On Sun, May 05, 2019 at 09:59:01PM +0200, Reimar Döffinger wrote: >> >> >>> +/*read video index*/ >>> +avio_seek(s->pb, 0xf8, SEEK_SET); >> [...] >>> +avio_skip(s->pb, ifv->vid_index->frame_offset - avio_tell(s->pb)); >> >> Why use avio_seek in one place and avio_skip in the other? >> > > No particular reason. Essentially all are just skips. There is no > backward seek. I left two seeks becuase they seemed more readable. > Someone could know at a glance at what offset the first video and audio > index are assumed/found to be. Should I change them to skips as well? Not quite sure how things work nowadays, but I'd suggest to use whichever gives the most readable code. Which would mean using avio_seek in this case. >>> +pos = avio_tell(s->pb); >>> + >>> +for (i = 0; i < ifv->total_vframes; i++) { >>> +e = &ifv->vid_index[i]; >>> +if (e->frame_offset >= pos) >>> +break; >>> +} >> >> This looks rather inefficient. >> Wouldn't it make more sense to either >> use a binary search or at least to >> remember the position from the last read? >> This also does not seem very robust either, >> if a single frame_offset gets corrupted >> to a very large value, this code will >> never be able to find the "correct" position. >> It seems to assume the frame_offset >> is ordered increasingly (as would be needed for >> binary search), but that property isn't >> really checked/enforced. >> > > Yeah it is indeed inefficient. But it also seems like the "correct" one. > Because in case of seeking we might not be at the boundary of a frame > and hence might need to skip to the boundary of next frame we can find. > I guess this rules out binary search, and maybe also saving the last > read. > > Regarding the frame_offset corruption, well that rules out binary search > as well because then the order of the index will be disturbed. > > Or maybe I misunderstood? Please do mention if this can be done more > efficiently by some method. I really need some ideas on this if it can > be done. First, seeking should be handled specially, by resetting the state. You should not make the get_packet less efficient because of that. That should enable the "remember last position and start from there". As to the corruption case, well the question is what to do about that, and I don't have the answer. But if the solution were to e.g. ensure the frame offset is monotonous then binary search could be used. However there is also the possibility that the format does in fact allow a completely arbitrary order of frames in the file, maybe even re-using an earlier frame_offset if the same frame appears multiple times. In that case this whole offset-based positioning code would simply be wrong, and you'd have to store the current index position in your demuxer instead of relying on avio_tell. Maybe you chose this solution because you did not know that seeking should be implemented via special functions? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avformat/ifv: added support for ifv cctv files
Am Mi., 8. Mai 2019 um 00:52 Uhr schrieb Reimar Döffinger : > > On 07.05.2019, at 12:00, Swaraj Hota wrote: > > > On Sun, May 05, 2019 at 09:59:01PM +0200, Reimar Döffinger wrote: > >> > >> > >>> +/*read video index*/ > >>> +avio_seek(s->pb, 0xf8, SEEK_SET); > >> [...] > >>> +avio_skip(s->pb, ifv->vid_index->frame_offset - avio_tell(s->pb)); > >> > >> Why use avio_seek in one place and avio_skip in the other? > >> > > > > No particular reason. Essentially all are just skips. There is no > > backward seek. I left two seeks becuase they seemed more readable. > > Someone could know at a glance at what offset the first video and audio > > index are assumed/found to be. Should I change them to skips as well? > > Not quite sure how things work nowadays, but I'd suggest to use whichever > gives the most readable code. > Which would mean using avio_seek in this case. I originally suggested using avio_skip() instead of avio_seek() to clarify that no back-seeking is involved. I realize it may not have been the best approach... Thank you for your comments, Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avformat/ifv: added support for ifv cctv files
On 08.05.2019, at 08:01, Carl Eugen Hoyos wrote: > Am Mi., 8. Mai 2019 um 00:52 Uhr schrieb Reimar Döffinger > : >> >> On 07.05.2019, at 12:00, Swaraj Hota wrote: >> >>> On Sun, May 05, 2019 at 09:59:01PM +0200, Reimar Döffinger wrote: > +/*read video index*/ > +avio_seek(s->pb, 0xf8, SEEK_SET); [...] > +avio_skip(s->pb, ifv->vid_index->frame_offset - avio_tell(s->pb)); Why use avio_seek in one place and avio_skip in the other? >>> >>> No particular reason. Essentially all are just skips. There is no >>> backward seek. I left two seeks becuase they seemed more readable. >>> Someone could know at a glance at what offset the first video and audio >>> index are assumed/found to be. Should I change them to skips as well? >> >> Not quite sure how things work nowadays, but I'd suggest to use whichever >> gives the most readable code. >> Which would mean using avio_seek in this case. > > I originally suggested using avio_skip() instead of avio_seek() to clarify > that no back-seeking is involved. > I realize it may not have been the best approach... Well, it is good advice in principle, but in this case it seems to lead to ugly code (and it was only done in some cases). That kind of thing tends to be hard to know before making the change. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avformat/ifv: added support for ifv cctv files
On Wed, May 08, 2019 at 12:52:01AM +0200, Reimar Döffinger wrote: > On 07.05.2019, at 12:00, Swaraj Hota wrote: > > > On Sun, May 05, 2019 at 09:59:01PM +0200, Reimar Döffinger wrote: > >> > >> > >>> +/*read video index*/ > >>> +avio_seek(s->pb, 0xf8, SEEK_SET); > >> [...] > >>> +avio_skip(s->pb, ifv->vid_index->frame_offset - avio_tell(s->pb)); > >> > >> Why use avio_seek in one place and avio_skip in the other? > >> > > > > No particular reason. Essentially all are just skips. There is no > > backward seek. I left two seeks becuase they seemed more readable. > > Someone could know at a glance at what offset the first video and audio > > index are assumed/found to be. Should I change them to skips as well? > > Not quite sure how things work nowadays, but I'd suggest to use whichever > gives the most readable code. > Which would mean using avio_seek in this case. > > >>> +pos = avio_tell(s->pb); > >>> + > >>> +for (i = 0; i < ifv->total_vframes; i++) { > >>> +e = &ifv->vid_index[i]; > >>> +if (e->frame_offset >= pos) > >>> +break; > >>> +} > >> > >> This looks rather inefficient. > >> Wouldn't it make more sense to either > >> use a binary search or at least to > >> remember the position from the last read? > >> This also does not seem very robust either, > >> if a single frame_offset gets corrupted > >> to a very large value, this code will > >> never be able to find the "correct" position. > >> It seems to assume the frame_offset > >> is ordered increasingly (as would be needed for > >> binary search), but that property isn't > >> really checked/enforced. > >> > > > > Yeah it is indeed inefficient. But it also seems like the "correct" one. > > Because in case of seeking we might not be at the boundary of a frame > > and hence might need to skip to the boundary of next frame we can find. > > I guess this rules out binary search, and maybe also saving the last > > read. > > > > Regarding the frame_offset corruption, well that rules out binary search > > as well because then the order of the index will be disturbed. > > > > Or maybe I misunderstood? Please do mention if this can be done more > > efficiently by some method. I really need some ideas on this if it can > > be done. > > First, seeking should be handled specially, by resetting the state. > You should not make the get_packet less efficient because of that. > That should enable the "remember last position and start from there". > > As to the corruption case, well the question is what to do about that, and I > don't have the answer. > But if the solution were to e.g. ensure the frame offset is monotonous then > binary search could be used. > However there is also the possibility that the format does in fact allow a > completely arbitrary order of frames in the file, maybe even re-using an > earlier frame_offset if the same frame appears multiple times. > In that case this whole offset-based positioning code would simply be wrong, > and you'd have to store the current index position in your demuxer instead of > relying on avio_tell. > Maybe you chose this solution because you did not know that seeking should be > implemented via special functions? By "special functions" do you mean those "read_seek" functions that are present in many demuxers(Cuz I have not implemented that)? If yes then am I mistaken that FFmpeg can also handle seeking automatically? (Carl suggesting something like that, iirc) Keeping the corruption case aside, how do you suggest I implement this? Is it not required to skip bytes till the next frame boundary in the read_packet function (as done in the current patch) ? If not then I guess I can go with a binary search, or better yet remember the last position. Thank you. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avformat/ifv: added support for ifv cctv files
On Wed, May 08, 2019 at 03:06:37PM +0530, Swaraj Hota wrote: > On Wed, May 08, 2019 at 12:52:01AM +0200, Reimar Döffinger wrote: > > First, seeking should be handled specially, by resetting the state. > > You should not make the get_packet less efficient because of that. > > That should enable the "remember last position and start from there". > > > > As to the corruption case, well the question is what to do about that, and > > I don't have the answer. > > But if the solution were to e.g. ensure the frame offset is monotonous then > > binary search could be used. > > However there is also the possibility that the format does in fact allow a > > completely arbitrary order of frames in the file, maybe even re-using an > > earlier frame_offset if the same frame appears multiple times. > > In that case this whole offset-based positioning code would simply be > > wrong, and you'd have to store the current index position in your demuxer > > instead of relying on avio_tell. > > Maybe you chose this solution because you did not know that seeking should > > be implemented via special functions? > > By "special functions" do you mean those "read_seek" functions that > are present in many demuxers(Cuz I have not implemented that)? Yes. > If yes then am I mistaken that FFmpeg can also handle seeking > automatically? (Carl suggesting something like that, iirc) It has some functionality, but I think in practice I don't think you can get a proper fully working solution with it. > Keeping the corruption case aside, how do you suggest I implement this? > Is it not required to skip bytes till the next frame boundary in the > read_packet function (as done in the current patch) ? > If not then I guess I can go with a binary search, or better yet > remember the last position. It's a bit more complex (in some aspect, simpler in others), but gxf.c might be an example. I will describe what I think would be a correct full implementation, but I will also note that maybe this is asking a bit much. You would start with changing your index reading function to use av_add_index_entry instead of storing your own index. In read_packet you simply remember your position in that index and use that information to know where to read from the next time. This should work for linear playback. Once that is done, read_seek would need implementing (it seems read_seek2 is currently only implemented by subtitle formats?). There you would seek the index for the right position (av_index_search_timestamp), and set the variables you use in read_packet accordingly so you will read the proper frames next. If the desired position is not in the index, you will also need to read the next index entries from the file (to start with you could do that by calling read_packet, even though it is a bit overkill). Unfortunately this is all a bit complicated and a lot of things that need to be implemented right. The "quick fix" would be to implement a read_seek function that only returns -1, with that you continue to use the current seeking code, but it would allow you to tell the read_packet function to use the "search index position based on file position" code, whereas for normal playback you just continue reading whatever you stored as "next index position to read". I'm sure far from everything I wrote is clear, but hopefully it gives you some starting points. Best regards, Reimar Döffinger ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v2] avformat/ifv: added support for ifv cctv files
On Wed, May 08, 2019 at 09:28:33PM +0200, Reimar Döffinger wrote: > On Wed, May 08, 2019 at 03:06:37PM +0530, Swaraj Hota wrote: > > On Wed, May 08, 2019 at 12:52:01AM +0200, Reimar Döffinger wrote: > > > First, seeking should be handled specially, by resetting the state. > > > You should not make the get_packet less efficient because of that. > > > That should enable the "remember last position and start from there". > > > > > > As to the corruption case, well the question is what to do about that, > > > and I don't have the answer. > > > But if the solution were to e.g. ensure the frame offset is monotonous > > > then binary search could be used. > > > However there is also the possibility that the format does in fact allow > > > a completely arbitrary order of frames in the file, maybe even re-using > > > an earlier frame_offset if the same frame appears multiple times. > > > In that case this whole offset-based positioning code would simply be > > > wrong, and you'd have to store the current index position in your demuxer > > > instead of relying on avio_tell. > > > Maybe you chose this solution because you did not know that seeking > > > should be implemented via special functions? > > > > By "special functions" do you mean those "read_seek" functions that > > are present in many demuxers(Cuz I have not implemented that)? > > Yes. > > > If yes then am I mistaken that FFmpeg can also handle seeking > > automatically? (Carl suggesting something like that, iirc) > > It has some functionality, but I think in practice I don't think > you can get a proper fully working solution with it. > > > Keeping the corruption case aside, how do you suggest I implement this? > > Is it not required to skip bytes till the next frame boundary in the > > read_packet function (as done in the current patch) ? > > If not then I guess I can go with a binary search, or better yet > > remember the last position. > > It's a bit more complex (in some aspect, simpler in others), but > gxf.c might be an example. > > I will describe what I think would be a correct full implementation, > but I will also note that maybe this is asking a bit much. > You would start with changing your index reading function to > use av_add_index_entry instead of storing your own index. > In read_packet you simply remember your position in that index and > use that information to know where to read from the next time. > This should work for linear playback. > > Once that is done, read_seek would need implementing > (it seems read_seek2 is currently only implemented by subtitle > formats?). > There you would seek the index for the right position > (av_index_search_timestamp), and set the variables you > use in read_packet accordingly so you will read the proper > frames next. > If the desired position is not in the index, you > will also need to read the next index entries from > the file (to start with you could do that by calling > read_packet, even though it is a bit overkill). > > Unfortunately this is all a bit complicated and a > lot of things that need to be implemented right. > The "quick fix" would be to implement a read_seek > function that only returns -1, with that you > continue to use the current seeking code, but > it would allow you to tell the read_packet > function to use the "search index position based > on file position" code, whereas for normal > playback you just continue reading whatever > you stored as "next index position to read". > > I'm sure far from everything I wrote is clear, > but hopefully it gives you some starting points. > > Best regards, > Reimar Döffinger That was really helpful! Thanks a lot! It is more or less clear to me now. Okay then I will surely try the "complicated" one first, and fallback to the "quick fix" otherwise. Really sorry if I am asking a lot of questions here ':D Swaraj ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".