| Message ID | 1510050836-28539-1-git-send-email-kjeyapal@akamai.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Nov 7, 2017 at 2:34 AM Karthick J <kjeyapal@akamai.com> wrote: > --- > libavformat/avio_internal.h | 8 ++++++++ > libavformat/aviobuf.c | 8 ++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/libavformat/avio_internal.h b/libavformat/avio_internal.h > index c01835d..04c1ad5 100644 > --- a/libavformat/avio_internal.h > +++ b/libavformat/avio_internal.h > @@ -133,6 +133,14 @@ int ffio_open_dyn_packet_buf(AVIOContext **s, int > max_packet_size); > int ffio_fdopen(AVIOContext **s, URLContext *h); > > /** > + * Return the URLContext associated with the AVIOContext > + * > + * @param s IO context > + * @return pointer to URLContext or NULL. > + */ > +URLContext *ffio_geturlcontext(AVIOContext *s); > + > +/** > * Open a write-only fake memory stream. The written data is not stored > * anywhere - this is only used for measuring the amount of data > * written. > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c > index 3b4c843..1353c80 100644 > --- a/libavformat/aviobuf.c > +++ b/libavformat/aviobuf.c > @@ -980,6 +980,14 @@ fail: > return AVERROR(ENOMEM); > } > > +URLContext* ffio_geturlcontext(AVIOContext *s) { > + AVIOInternal *internal = s->opaque; > + if (internal) > + return internal->h; > + else > + return NULL; > +} > + > int ffio_ensure_seekback(AVIOContext *s, int64_t buf_size) > { > uint8_t *buffer; LGTM. This would make my hls demuxer keepalive patch simpler as well. I know there were some concerns earlier about exposing URLContext, but since this is internal I think it should be okay. Any objections? Aman > -- > 1.9.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
>>On Tue, Nov 7, 2017 at 2:34 AM Karthick J <kjeyapal@akamai.com> wrote: >>--- >> libavformat/avio_internal.h | 8 ++++++++ >> libavformat/aviobuf.c | 8 ++++++++ >> 2 files changed, 16 insertions(+) > >LGTM. This would make my hls demuxer keepalive patch simpler as well. > >I know there were some concerns earlier about exposing URLContext, but since this is internal I think it should be okay. > >Any objections? > >Aman Well more than a week has passed by. It looks like there are no objections from anyone. If that is the case, could somebody push this patchset to mainline. I don’t have push permissions. Regards, Karthick
Le septidi 17 brumaire, an CCXXVI, Karthick J a écrit : > --- > libavformat/avio_internal.h | 8 ++++++++ > libavformat/aviobuf.c | 8 ++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/libavformat/avio_internal.h b/libavformat/avio_internal.h > index c01835d..04c1ad5 100644 > --- a/libavformat/avio_internal.h > +++ b/libavformat/avio_internal.h > @@ -133,6 +133,14 @@ int ffio_open_dyn_packet_buf(AVIOContext **s, int max_packet_size); > int ffio_fdopen(AVIOContext **s, URLContext *h); > > /** > + * Return the URLContext associated with the AVIOContext > + * > + * @param s IO context > + * @return pointer to URLContext or NULL. > + */ > +URLContext *ffio_geturlcontext(AVIOContext *s); > + > +/** > * Open a write-only fake memory stream. The written data is not stored > * anywhere - this is only used for measuring the amount of data > * written. > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c > index 3b4c843..1353c80 100644 > --- a/libavformat/aviobuf.c > +++ b/libavformat/aviobuf.c > @@ -980,6 +980,14 @@ fail: > return AVERROR(ENOMEM); > } > > +URLContext* ffio_geturlcontext(AVIOContext *s) { > + AVIOInternal *internal = s->opaque; > + if (internal) > + return internal->h; This is rather fragile. Some kind of check that is is actually a real URLContext would be a very good idea. > + else > + return NULL; > +} > + > int ffio_ensure_seekback(AVIOContext *s, int64_t buf_size) > { > uint8_t *buffer; Regards,
>On 11/15/17, 6:34 PM, "Nicolas George" <george@nsup.org> wrote: >This is rather fragile. Some kind of check that is is actually a real >URLContext would be a very good idea. Thanks for the feedback. I have a relevant condition check so that only a real URLContext is returned. Please find the new patch attached. Regards, Karthick >Regards, > >-- > Nicolas George
Le sextidi 26 brumaire, an CCXXVI, Jeyapal, Karthick a écrit : > Thanks for the feedback. I have a relevant condition check so that > only a real URLContext is returned. > Please find the new patch attached. I think that would work, thanks. But I see that in the other patches you call ffio_geturlcontext() without checking its return value. That is not good at all, and that would make Coverity squirm. If you are sure that you are always calling it with a real URLContext, then make the test an av_assert0(). I think it is the case. Otherwise, you need to check the return value. Also, I see the other patch calling prot->url_write directly: I think it should call ffurl_write() instead. Regards,
>On 11/16/17, 4:43 PM, "Nicolas George" <george@nsup.org> wrote: Thanks for the reply. >I think that would work, thanks. > >But I see that in the other patches you call ffio_geturlcontext() >without checking its return value. That is not good at all, and that >would make Coverity squirm. > >If you are sure that you are always calling it with a real URLContext, >then make the test an av_assert0(). I think it is the case. Otherwise, >you need to check the return value. I have done the change as suggested. Please find the new patch attached. >Also, I see the other patch calling prot->url_write directly: I think it >should call ffurl_write() instead. I need to send ‘0’ byte http packet to indicate close. But ffurl_write doesn’t call http_write for a 0 byte packet. Hence, I am calling prot->url_write directly. >Regards, > >-- > Nicolas George Regards, Karthick
Le sextidi 26 brumaire, an CCXXVI, Jeyapal, Karthick a écrit : > I have done the change as suggested. Please find the new patch attached. Sorry, but I still have doubts about it. To begin with, -1 is not an acceptable error code. But what bothers me most is that on the two uses, one has an assert, the other has an error return, the inconsistency looks suspicious. I think, at least, it should include a comment at both places to indicate why the test is done like that. > I need to send ‘0’ byte http packet to indicate close. > But ffurl_write doesn’t call http_write for a 0 byte packet. > Hence, I am calling prot->url_write directly. Let me see if I understand correctly: are you saying that it is not possible to perform multiple requests using only public APIs? I find it suspicious, since multiple_requests is a public option. If that is true, then it needs fixing. Regards,
>On 11/17/17, 4:31 PM, "Nicolas George" <george@nsup.org> wrote: > >Le sextidi 26 brumaire, an CCXXVI, Jeyapal, Karthick a écrit : >> I have done the change as suggested. Please find the new patch attached. > >Sorry, but I still have doubts about it. > >To begin with, -1 is not an acceptable error code. > >But what bothers me most is that on the two uses, one has an assert, the >other has an error return, the inconsistency looks suspicious. > >I think, at least, it should include a comment at both places to >indicate why the test is done like that. > >> I need to send ‘0’ byte http packet to indicate close. >> But ffurl_write doesn’t call http_write for a 0 byte packet. >> Hence, I am calling prot->url_write directly. > >Let me see if I understand correctly: are you saying that it is not >possible to perform multiple requests using only public APIs? I find it >suspicious, since multiple_requests is a public option. If that is true, >then it needs fixing. Thanks for your reply. I have sent a new patch set v2 addressing your concerns. Yes, http multiple_requests option is not working as expected. Few days back a fix was done for read http://ffmpeg.org/pipermail/ffmpeg-devel/2017-November/219746.html As per your suggestion, I have added fix to the public api of http for write. Now hlsenc does not call prot->url_write directly in this new patch set. Thanks and regards, Karthick
diff --git a/libavformat/avio_internal.h b/libavformat/avio_internal.h index c01835d..04c1ad5 100644 --- a/libavformat/avio_internal.h +++ b/libavformat/avio_internal.h @@ -133,6 +133,14 @@ int ffio_open_dyn_packet_buf(AVIOContext **s, int max_packet_size); int ffio_fdopen(AVIOContext **s, URLContext *h); /** + * Return the URLContext associated with the AVIOContext + * + * @param s IO context + * @return pointer to URLContext or NULL. + */ +URLContext *ffio_geturlcontext(AVIOContext *s); + +/** * Open a write-only fake memory stream. The written data is not stored * anywhere - this is only used for measuring the amount of data * written. diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c index 3b4c843..1353c80 100644 --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -980,6 +980,14 @@ fail: return AVERROR(ENOMEM); } +URLContext* ffio_geturlcontext(AVIOContext *s) { + AVIOInternal *internal = s->opaque; + if (internal) + return internal->h; + else + return NULL; +} + int ffio_ensure_seekback(AVIOContext *s, int64_t buf_size) { uint8_t *buffer;