Re: [webkit-dev] [MSE] Range ends inclusion is deleting wanted MediaSample's
On 14/11/17 22:55, Jer Noble wrote: > > Yeah, the spec is not the best here. I agree. While WebKit replicates the specified steps faithfully, it's not clear they are good steps in the first place. I spent quite a bit of time trying to wrap my head around the three (!) frame removal cases, all of them apparently doing the same but with slightly different ranges and being used in slightly different situations... Even after all the complexity I found too some cases that are unhandled by the spec. > If you have, e.g. a sample with a > PTS of 0 and a duration of 100, and then insert a sample with a PTS of > 50 and a duration of 100, you’d expect that to cause the first sample to > be removed. But a strict reading of the spec says that sample stays. Or maybe cropped (that would be of questionable utility in practice though)... But yup, the spec says that (supposing they are consecutive) no check at all is performed because PTS < highestEndTimestamp. > Now you have two overlapping samples. It can get even weirder if you > insert a sample with a PTS of 25 and a duration of 50. Now, strictly > implementing the spec, you have a sample overlaps on both ends of > another sample. What does that even mean for a decoder? It’s almost > guaranteed to generate a decode error, unless both of the overlapping > samples are I-frames. It would be implementation specific. In the case of GStreamer, for instance, durations are not needed for decoders to work, only DTS. Same can be said for the presentation, that only needs to know the total length of the movie (or at the very least know whether it should continue playing) and the PTS of each frame. So the first frame would be shown for 25 ms, then the second one for 75 ms. No corruption should occur as the frames have been decoded in the correct order. More serious problems can arise from manipulating the array of frames as many assumptions would be broken: frames would be played for less than their declared durations, the relation "time to frame" would no longer have 0..1 multiplicity, the sum of durations would be longer than the last frame end even though they are thought to be consecutive and the first frame has PTS=0 and so on... Definitively undesirable consequences independently of the platform. > > I think the intent of the spec is clear: if any part of a previous > sample overlaps the new one, it has to be removed, and all samples that > depend on the removed samples must be removed as well. And in order to > do that, you have to take the entire duration of the sample into account. > I wish that part of the MSE spec was rewritten in a simpler yet more comprehensive way... because as of today, that's not what the spec says. Some complexity may need to appear nevertheless because of the float conversions caused by JS interaction. -- Alicia. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] [MSE] Range ends inclusion is deleting wanted MediaSample's
> On Nov 14, 2017, at 1:36 PM, Alicia Boya García wrote: > > Hi, Jer. > > Sorry for the late reply, I've been busy investigating all of this. > > I'm replying to your points in reverse order... > > On 09/11/17 23:23, Jer Noble wrote: >>> As I understand it, the point of that part of the algorithm is to delete >>> old samples that are -- even partially -- in the presentation time range >>> of the newly appended one, but using (beginTime, endTime] fails to >>> accomplish that in two cases: >>> >>> a) If there is an existing MediaSample with PTS=9 and DUR=1 it will not >>> be removed because beginTime (=9) is exclusive. >> >> Eh, not really. The search space is the entire duration of the sample, >> so an exclusive beginTime of 9 _should_ match PTS=9, DUR=1 because 9 < >> presentationEndTime=10. I’m pretty sure this is correct, but a mock >> test case should clear this up pretty quickly. > > That's totally not the case, though the first time I gave a look at this > code I thought that too... presentationRange() is a std::map whose > key type is a single MediaTime specifying the PTS of the MediaSample, > not the duration or the full presentation time range. > > Also, neither findSamplesWithinPresentationRange() or its siblings take > durations into account, as they search only with the key (PTS) in > mind... Well, actually findSamplesWithinPresentationRangeFromEnd() looks > at the values, but it only checks .presentationTimestamp() so it's > functionally the same. Aha, okay. There’s also SampleIsLessThanMediaTimeComparator, SampleIsGreaterThanMediaTimeComparator, which does take duration into account, but those look to be unused. > Adding a sample with same PTS as an existing one does not remove the old > one with this check. There are two possible behaviors that can mask the > issue: > > a) The previous frame removal step (see step 1.13 in the spec or 1.14 in > WebKit) removed the overlapping frame. This is only the case if the > frame started a new Coded Frame Group. > > b) The existing overlapping frame was not removed, but later on when the > new MediaSample was passed to std::map::insert() it was ignored because > it was a duplicated key (same PTS as the existing overlapping frame). > > You can confirm that it's the old MediaSample the one persisted by > setting different durations for the old and the new one but the same > PTS. After the append of the second frame, the old duration persists and > there are no new frames in the TrackBuffer. > > On 09/11/17 23:23, Jer Noble wrote: >>> My question is... shouldn't the range ends inclusivity be the other way >>> around i.e. [beginTime, endTime)? >> >> beginTime was intended to be exclusive so that, given a [0, 9] range, >> searching for (9, 10] wouldn’t match the last sample in the [0, 9] range. >> >> But now that you mention it, perhaps the real problem is that both beginTime >> _and_ endTime should be exclusive. > > This is not an issue because as explained before we are considering just > the frame start PTS, not its entire presentation interval. I think we should consider the entire interval, though. See below: > beginTime must be inclusive so that old frames with the same PTS are > deleted. endTime must be exclusive so that non overlapping consecutive > frames are not deleted. > > I have checked this with the spec: > https://www.w3.org/TR/media-source/#sourcebuffer-coded-frame-processing > > 1.14. Remove existing coded frames in track buffer: > -> If highest end timestamp for track buffer is not set: > [...] > -> If highest end timestamp for track buffer is set and less than or >equal to presentation timestamp: > Remove all coded frames from track buffer that have a > presentation timestamp greater than or equal to highest end > timestamp and less than frame end timestamp. Yeah, the spec is not the best here. If you have, e.g. a sample with a PTS of 0 and a duration of 100, and then insert a sample with a PTS of 50 and a duration of 100, you’d expect that to cause the first sample to be removed. But a strict reading of the spec says that sample stays. Now you have two overlapping samples. It can get even weirder if you insert a sample with a PTS of 25 and a duration of 50. Now, strictly implementing the spec, you have a sample overlaps on both ends of another sample. What does that even mean for a decoder? It’s almost guaranteed to generate a decode error, unless both of the overlapping samples are I-frames. I think the intent of the spec is clear: if any part of a previous sample overlaps the new one, it has to be removed, and all samples that depend on the removed samples must be removed as well. And in order to do that, you have to take the entire duration of the sample into account. > On 09/11/17 23:23, Jer Noble wrote: >> It should be possible to make a “mock” testcase which doesn’t rely on >> any particular media engine and which can demonstrate this bug. > > I just fini
Re: [webkit-dev] [MSE] Range ends inclusion is deleting wanted MediaSample's
Hi, Jer. Sorry for the late reply, I've been busy investigating all of this. I'm replying to your points in reverse order... On 09/11/17 23:23, Jer Noble wrote: >> As I understand it, the point of that part of the algorithm is to delete >> old samples that are -- even partially -- in the presentation time range >> of the newly appended one, but using (beginTime, endTime] fails to >> accomplish that in two cases: >> >> a) If there is an existing MediaSample with PTS=9 and DUR=1 it will not >> be removed because beginTime (=9) is exclusive. > > Eh, not really. The search space is the entire duration of the sample, > so an exclusive beginTime of 9 _should_ match PTS=9, DUR=1 because 9 < > presentationEndTime=10. I’m pretty sure this is correct, but a mock > test case should clear this up pretty quickly. That's totally not the case, though the first time I gave a look at this code I thought that too... presentationRange() is a std::map whose key type is a single MediaTime specifying the PTS of the MediaSample, not the duration or the full presentation time range. Also, neither findSamplesWithinPresentationRange() or its siblings take durations into account, as they search only with the key (PTS) in mind... Well, actually findSamplesWithinPresentationRangeFromEnd() looks at the values, but it only checks .presentationTimestamp() so it's functionally the same. Adding a sample with same PTS as an existing one does not remove the old one with this check. There are two possible behaviors that can mask the issue: a) The previous frame removal step (see step 1.13 in the spec or 1.14 in WebKit) removed the overlapping frame. This is only the case if the frame started a new Coded Frame Group. b) The existing overlapping frame was not removed, but later on when the new MediaSample was passed to std::map::insert() it was ignored because it was a duplicated key (same PTS as the existing overlapping frame). You can confirm that it's the old MediaSample the one persisted by setting different durations for the old and the new one but the same PTS. After the append of the second frame, the old duration persists and there are no new frames in the TrackBuffer. On 09/11/17 23:23, Jer Noble wrote: >> My question is... shouldn't the range ends inclusivity be the other way >> around i.e. [beginTime, endTime)? > > beginTime was intended to be exclusive so that, given a [0, 9] range, > searching for (9, 10] wouldn’t match the last sample in the [0, 9] range. > > But now that you mention it, perhaps the real problem is that both beginTime > _and_ endTime should be exclusive. This is not an issue because as explained before we are considering just the frame start PTS, not its entire presentation interval. beginTime must be inclusive so that old frames with the same PTS are deleted. endTime must be exclusive so that non overlapping consecutive frames are not deleted. I have checked this with the spec: https://www.w3.org/TR/media-source/#sourcebuffer-coded-frame-processing 1.14. Remove existing coded frames in track buffer: -> If highest end timestamp for track buffer is not set: [...] -> If highest end timestamp for track buffer is set and less than or equal to presentation timestamp: Remove all coded frames from track buffer that have a presentation timestamp greater than or equal to highest end timestamp and less than frame end timestamp. On 09/11/17 23:23, Jer Noble wrote: > It should be possible to make a “mock” testcase which doesn’t rely on > any particular media engine and which can demonstrate this bug. I just finished doing that. I have written two new MediaSource tests using video/mock: one that demonstrates how existing frames whose PTS coincide with the start of the range of the range are not deleted and another one that demonstrates how existing frames whose PTS coincide with the end of the range are deleted. I have also written a patch with the correct checks: https://bugs.webkit.org/show_bug.cgi?id=179690 -- Alicia. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] unified sources build + forwarding headers that are copies
Our CMakeLists.txt have many instances of checks like “if (WIN32)” that assume that if you are running CMake on Windows then you are building for Windows. If you can make these checks work for you without breaking the existing Windows builds, then we would welcome such improvements. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] unified sources build + forwarding headers that are copies
14.11.2017, 19:10, "Michael Catanzaro" : > On Tue, Nov 14, 2017 at 5:54 AM, Salisbury, Mark > wrote: >> Hello, >> >> I’m building on Windows (WPE platform, perhaps I’m the first to >> try this!) > > Wow! > >> and I’m seeing some compile errors that I believe are arising from >> previously untested combinations of source files. >> >> If I understand unified sources build correctly, it uses a large list >> of files and then arranges them into groups, 8 files at a time. If I >> have a slightly different set of files than is standard, or if >> someone adds a new source file, the combinations of files can change. > > Well, of course the combination of files can change if someone adds a > new file. But the cross-platform files that get combined together > should always be the same for all ports, because the bundles defined in > Sources.txt are deterministic. Only files from the same directory get > bundled together (they're separated by a blank line in Sources.txt) and > if multiple bundles are used for the same directory, they should always > be the same on all ports. > > (Of course, you can also have port-specific Sources.txt files.) > >> Do non-windows ports use a more intelligent method (a pointer, not a >> file copy) for creating forwarding headers for JavaScriptCore >> includes? > > Yes, non-Windows ports #include the original file instead of copying > the original. E.g. in > DerivedSources/ForwardingHeaders/JavaScriptCore/ArrayBufferSharingMode.h, > I have the following one-liner: > > #include "JavaScriptCore/runtime/ArrayBufferSharingMode.h" > > I'm not sure why Windows is different! See the > WEBKIT_CREATE_FORWARDING_HEADERS macro in > Source/cmake/WebKitMacros.cmake. AFAIK Windows uses different approach because AppleWin needs to support separate building of WTF, WebCore, and WebKit. FWIW Qt port uses same handling of forwarding headers on all platforms. > >> Is the windows build the only one to use unified sources currently? > > No, all ports are using it. > > Michael > > ___ > webkit-dev mailing list > webkit-dev@lists.webkit.org > https://lists.webkit.org/mailman/listinfo/webkit-dev -- Regards, Konstantin ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] unified sources build + forwarding headers that are copies
On Tue, Nov 14, 2017 at 5:54 AM, Salisbury, Mark wrote: Hello, I’m building on Windows (WPE platform, perhaps I’m the first to try this!) Wow! and I’m seeing some compile errors that I believe are arising from previously untested combinations of source files. If I understand unified sources build correctly, it uses a large list of files and then arranges them into groups, 8 files at a time. If I have a slightly different set of files than is standard, or if someone adds a new source file, the combinations of files can change. Well, of course the combination of files can change if someone adds a new file. But the cross-platform files that get combined together should always be the same for all ports, because the bundles defined in Sources.txt are deterministic. Only files from the same directory get bundled together (they're separated by a blank line in Sources.txt) and if multiple bundles are used for the same directory, they should always be the same on all ports. (Of course, you can also have port-specific Sources.txt files.) Do non-windows ports use a more intelligent method (a pointer, not a file copy) for creating forwarding headers for JavaScriptCore includes? Yes, non-Windows ports #include the original file instead of copying the original. E.g. in DerivedSources/ForwardingHeaders/JavaScriptCore/ArrayBufferSharingMode.h, I have the following one-liner: #include "JavaScriptCore/runtime/ArrayBufferSharingMode.h" I'm not sure why Windows is different! See the WEBKIT_CREATE_FORWARDING_HEADERS macro in Source/cmake/WebKitMacros.cmake. Is the windows build the only one to use unified sources currently? No, all ports are using it. Michael ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] unified sources build + forwarding headers that are copies
Hello, I'm building on Windows (WPE platform, perhaps I'm the first to try this!) and I'm seeing some compile errors that I believe are arising from previously untested combinations of source files. If I understand unified sources build correctly, it uses a large list of files and then arranges them into groups, 8 files at a time. If I have a slightly different set of files than is standard, or if someone adds a new source file, the combinations of files can change. If the same header file is included twice (and forwarding header file is a copy of the original), this causes duplicate definitions / compile errors. My question is - is there a right / ideal way to fix these conflicts? d:\git\webkit-org\source\javascriptcore\runtime\ArrayBufferSharingMode.h is included twice, the first time, via the real path, the second time, via a JavaScriptCore forwarding header. Note that this forwarding header is a copy of the header file, not a pointer/alias to the real header path. The code that creates the forwarding header version comes from D:\git\webkit-org\Source\JavaScriptCore\PlatformWin.cmake (see xcopy around line 40). Do non-windows ports use a more intelligent method (a pointer, not a file copy) for creating forwarding headers for JavaScriptCore includes? Is the windows build the only one to use unified sources currently? Source/WebKit/Scripts/generate-forwarding-headers.pl scans .c|.cpp files for forwarding headers that need to be created and creates a header 'pointer' to the real path, so both paths could be included (forwarding header vs. real) and there would be no conflict, because ultimately, only the real header is included. So I guess I'm suggesting to use a forwarding header 'pointer' instead of header file xcopy, which would mean running a script that recurses directories in javascriptcore and creates forwarding headers. That would mean no duplicate header file name would be allowed in JavaScriptCore - I don't think that's a problem though. (I don't really like the idea of grep'ing all webcore to create forwarding headers where needed, grep'ing webcore gets expensive) . Just wondering if there are other suggestions. Thanks, Mark (Personally I don't like forwarding headers but that's another topic... not sure if the benefits outweigh associated problems) D:\git\webkit-org\WebKitBuild\Debug\DerivedSources\WebCore\unified-sources\UnifiedSource180.cpp Turned on 'show includes', you can see the two ways I get to the same header file: First source file including ArrayBufferSharingMode.h: 1>Note: including file: D:\git\webkit-org\Source\WebCore\css/DOMMatrix.cpp 1>Note: including file: D:\git\webkit-org\Source\WebCore\config.h 1>Note: including file: d:\git\webkit-org\source\webcore\css\DOMMatrix.h 1>Note: including file: d:\git\webkit-org\source\webcore\css\DOMMatrixReadOnly.h 1>Note: including file:d:\git\webkit-org\source\webcore\css\DOMMatrixInit.h 1>Note: including file: d:\git\webkit-org\source\webcore\css\DOMMatrix2DInit.h 1>Note: including file: D:\git\webkit-org\Source\WebCore\bindings\js\ScriptWrappable.h 1>Note: including file: D:\git\webkit-org\Source\JavaScriptCore\heap/Weak.h 1>Note: including file: D:\git\webkit-org\Source\WebCore\platform\graphics\transforms\TransformationMatrix.h 1>Note: including file: D:\git\webkit-org\Source\WebCore\platform\graphics\FloatPoint.h 1>Note: including file: d:\git\webkit-org\source\webcore\platform\graphics\FloatSize.h 1>Note: including file: d:\git\webkit-org\source\webcore\platform\graphics\IntPoint.h 1>Note: including file: d:\git\webkit-org\source\webcore\platform\graphics\IntSize.h 1>Note: including file: D:\git\webkit-org\Source\WebCore\platform\graphics\FloatPoint3D.h 1>Note: including file: D:\git\webkit-org\Source\JavaScriptCore\runtime/Float32Array.h 1>Note: including file: d:\git\webkit-org\source\javascriptcore\runtime\TypedArrays.h 1>Note: including file: d:\git\webkit-org\source\javascriptcore\runtime\GenericTypedArrayView.h 1>Note: including file: d:\git\webkit-org\source\javascriptcore\runtime\ArrayBuffer.h 1>Note: including file: d:\git\webkit-org\source\javascriptcore\runtime\ArrayBufferSharingMode.h Second source file including ArrayBufferSharingMode.h: 1>Note: including file: D:\git\webkit-org\Source\WebCore\css/DOMMatrixReadOnly.cpp 1>Note: including file: D:\git\webkit-org\Source\WebCore\config.h 1>Note: including file: d:\git\webkit-org\source\webcore\css\CSSToLengthConversionData.h 1>Note: including file: D:\git\webkit-org\Source\WebCore\dom\DOMPoint.h 1>Note: including file: d:\git\webkit-org\source\webcore\dom\DOMPointReadOnly.h 1>Note: including file:d:\git\webkit-org\source\webcore\dom\DOMPointInit.h 1>Note: including file: d:\git\webkit-org\source\webcore\css\TransformFunctions.h 1>Note: including file: D:\git\webkit-org\Source\WebCore\platform\graphics\transforms\TransformOpera
Re: [webkit-dev] [webkit-changes] [224805] trunk/Source/WebKit
Sure, __MAC_OS_X_VERSION_MIN_REQUIRED < 101300 is what I intended. It worked just fine on macOS Sierra 16G29 however. - R. Niwa On Mon, Nov 13, 2017 at 11:42 PM, Dan Bernstein wrote: > > > On Nov 13, 2017, at 11:26 PM, rn...@webkit.org wrote: > > -#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101200+#if > __MAC_OS_X_VERSION_MIN_REQUIRED <= 101200 > > A right-closed interval for __MAC_OS_X_VERSION_MIN_REQUIRED is usually > incorrect. Did you mean to write < 101300 ? > ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
