| Message ID | 20230514194129.18840-1-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] Revert "avformat/hls: fail on probing non hls/m3u8 file extensions" | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Sun, May 14, 2023 at 09:41:29PM +0200, Anton Khirnov wrote: > This reverts commit 6b1f68ccb04d791f0250e05687c346a99ff47ea1, which > broke many streams in the wild > > Fixes #10353. This change violates a SHOULD in rfc8216 4. Playlists Each Playlist file MUST be identifiable either by the path component of its URI or by HTTP Content-Type. In the first case, the path MUST end with either .m3u8 or .m3u. In the second, the HTTP Content-Type MUST be "application/vnd.apple.mpegurl" or "audio/mpegurl". Clients SHOULD refuse to parse Playlists that are not so identified. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ The commit message should mention the RFC and why it is not following the recommandition of not parsing such Playlists Also ive taken a very brief look at #10353 and the example there shown seems to fail because of the "?..." stuff in the url, it seemed to be .m3u8 files, only the leaves seemes .ts I do not object to revert this as a temporary fix of course thx [...]
On 5/14/23 17:39, Michael Niedermayer wrote: > On Sun, May 14, 2023 at 09:41:29PM +0200, Anton Khirnov wrote: >> This reverts commit 6b1f68ccb04d791f0250e05687c346a99ff47ea1, which >> broke many streams in the wild >> >> Fixes #10353. > > This change violates a SHOULD in rfc8216 4. Playlists > > Each Playlist file MUST be identifiable either by the path component > of its URI or by HTTP Content-Type. In the first case, the path MUST > end with either .m3u8 or .m3u. In the second, the HTTP Content-Type > MUST be "application/vnd.apple.mpegurl" or "audio/mpegurl". > > Clients SHOULD refuse to parse Playlists that are not so identified. > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Except that commit doesn't check the mimetype. Some CDNs such as Azure Media Services, only use mimetype to identify HLS streams, and not file extensions. See: https://learn.microsoft.com/en-us/azure/media-services/latest/encode-dynamic-packaging-concept This commit breaks more than it helps. - Leo Izen (Traneptora / thebombzen)
On Sun, 14 May 2023 at 23:39, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Sun, May 14, 2023 at 09:41:29PM +0200, Anton Khirnov wrote: > > This reverts commit 6b1f68ccb04d791f0250e05687c346a99ff47ea1, which > > broke many streams in the wild > > > > Fixes #10353. > > This change violates a SHOULD in rfc8216 4. Playlists > > Each Playlist file MUST be identifiable either by the path component > of its URI or by HTTP Content-Type. either/or > In the first case, the path MUST > end with either .m3u8 or .m3u. First case, path component with valid extention. > In the second, the HTTP Content-Type > MUST be "application/vnd.apple.mpegurl" or "audio/mpegurl". Second (OR) case for URI with Content-Type. So current FFmpeg HEAD violate RFC, as it should allow URI with Content-Type. - Kacper
On Mon, May 15, 2023 at 12:40:50AM +0200, Kacper Michajlow wrote: > On Sun, 14 May 2023 at 23:39, Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Sun, May 14, 2023 at 09:41:29PM +0200, Anton Khirnov wrote: > > > This reverts commit 6b1f68ccb04d791f0250e05687c346a99ff47ea1, which > > > broke many streams in the wild > > > > > > Fixes #10353. > > > > This change violates a SHOULD in rfc8216 4. Playlists > > > > Each Playlist file MUST be identifiable either by the path component > > of its URI or by HTTP Content-Type. > > either/or > > > In the first case, the path MUST > > end with either .m3u8 or .m3u. > > First case, path component with valid extention. > > > In the second, the HTTP Content-Type > > MUST be "application/vnd.apple.mpegurl" or "audio/mpegurl". > > Second (OR) case for URI with Content-Type. > > So current FFmpeg HEAD violate RFC, as it should allow URI with Content-Type. yes, code is buggy, i didnt see teh report before today night, and was doing too much non ffmpeg crap last 2-3 days so didnt see it before everyone is upset, noone mailed me, noone posted a good fix, noone reverted ill post something before going to sleep today probably but i likely wont have the time to properly test it before bed (testcases with ffmpeg / ffplay btw makes it easier to test than with links to random webpages) thx [...]
On Mon, 15 May 2023 at 01:06, Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Mon, May 15, 2023 at 12:40:50AM +0200, Kacper Michajlow wrote: > > On Sun, 14 May 2023 at 23:39, Michael Niedermayer > > <michael@niedermayer.cc> wrote: > > > > > > On Sun, May 14, 2023 at 09:41:29PM +0200, Anton Khirnov wrote: > > > > This reverts commit 6b1f68ccb04d791f0250e05687c346a99ff47ea1, which > > > > broke many streams in the wild > > > > > > > > Fixes #10353. > > > > > > This change violates a SHOULD in rfc8216 4. Playlists > > > > > > Each Playlist file MUST be identifiable either by the path component > > > of its URI or by HTTP Content-Type. > > > > either/or > > > > > In the first case, the path MUST > > > end with either .m3u8 or .m3u. > > > > First case, path component with valid extention. > > > > > In the second, the HTTP Content-Type > > > MUST be "application/vnd.apple.mpegurl" or "audio/mpegurl". > > > > Second (OR) case for URI with Content-Type. > > > > So current FFmpeg HEAD violate RFC, as it should allow URI with Content-Type. > > yes, code is buggy, i didnt see teh report before today night, > and was doing too much non ffmpeg crap last 2-3 days so didnt see it before > > everyone is upset, noone mailed me, noone posted a good fix, noone reverted > > ill post something before going to sleep today probably but i likely > wont have the time to properly test it before bed > (testcases with ffmpeg / ffplay btw makes it easier to test than with > links to random webpages) > > thx > All good, take your time. I just wanted to clarify that current implementation is too strict/buggy. I think the upset comes from the fact that a lot of users started flooding issue trackers with the complaints that their favorite live stream doesn't work. And while the underlying issue is quite small regression, there were uncertainty at first what is going on and a lot of unnecessary comments and remarks were made. - Kacper
Quoting Michael Niedermayer (2023-05-14 23:39:44) > On Sun, May 14, 2023 at 09:41:29PM +0200, Anton Khirnov wrote: > > This reverts commit 6b1f68ccb04d791f0250e05687c346a99ff47ea1, which > > broke many streams in the wild > > > > Fixes #10353. > > This change violates a SHOULD in rfc8216 4. Playlists > > Each Playlist file MUST be identifiable either by the path component > of its URI or by HTTP Content-Type. In the first case, the path MUST > end with either .m3u8 or .m3u. In the second, the HTTP Content-Type > MUST be "application/vnd.apple.mpegurl" or "audio/mpegurl". > > Clients SHOULD refuse to parse Playlists that are not so identified. > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > The commit message should mention the RFC and why it is not following the > recommandition of not parsing such Playlists It's been a longstanding project policy to try to process non-strictly compliant files when possible. And as I've said in the other thread (did not have time yet to reply there, hopefully will soon), I don't think this is the place to enforce such policies. The fact that you have to use NULL logging context is by itself indicative of that.
diff --git a/libavformat/hls.c b/libavformat/hls.c index 11e345b280..8a96a37ff9 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -2532,15 +2532,8 @@ static int hls_probe(const AVProbeData *p) if (strstr(p->buf, "#EXT-X-STREAM-INF:") || strstr(p->buf, "#EXT-X-TARGETDURATION:") || - strstr(p->buf, "#EXT-X-MEDIA-SEQUENCE:")) { - - if (!av_match_ext(p->filename, "m3u8,hls,m3u")) { - av_log(NULL, AV_LOG_ERROR, "Not detecting m3u8/hls with non standard extension\n"); - return 0; - } - + strstr(p->buf, "#EXT-X-MEDIA-SEQUENCE:")) return AVPROBE_SCORE_MAX; - } return 0; }