Parse error on valid SVG

[zathras]

The attached SVG validated as a correctly-formatted UTF-8 encoded SVG 1.1 document according to https://validator.w3.org/check , but it generates the following error:

======== Exception caught by SVG ===================================================================
The following StateError was thrown resolving a single-frame picture stream:
Bad state: Expected to find Drawable with id url(#path22603).
Have ids: (url(#svg2), url(#club_1), url(#rect9340))

When the exception was thrown, this was the stack: 
#0      DrawableDefinitionServer.getDrawable (package:flutter_svg/src/vector_drawable.dart:568:7)
#1      _Elements.use (package:flutter_svg/src/svg/parser_state.dart:201:34)
#2      SvgParserState.parse (package:flutter_svg/src/svg/parser_state.dart:808:26)
<asynchronous suspension>
#3      SvgParser.parse (package:flutter_svg/parser.dart:25:12)
...
Picture provider: ExactAssetPicture(name: "assets/cards/anglo.svg", bundle: null, colorFilter: null)
Picture key: AssetBundlePictureKey(bundle: PlatformAssetBundle#c5618(), name: "assets/cards/anglo.svg", colorFilter: null)
====================================================================================================

Reproduced with:

void main() {
  runApp(MaterialApp(title: 'foo', home: SvgPicture.asset('assets/cards/anglo.svg')));
}

FWIW, this is on macos desktop...

billf@londo:~/github/jrpn/docs$ flutter doctor
Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel stable, 2.0.6, on macOS 11.2.3 20D91 darwin-arm, locale
    en-US)
[!] Android toolchain - develop for Android devices (Android SDK version 30.0.3)
    ✗ Android license status unknown.
      Run `flutter doctor --android-licenses` to accept the SDK licenses.
      See https://flutter.dev/docs/get-started/install/macos#android-setup for
      more details.
[✓] Xcode - develop for iOS and macOS
[✓] Chrome - develop for the web
[✓] Android Studio (version 4.2)
[✓] IntelliJ IDEA Community Edition (version 2021.1)
[✓] Connected device (2 available)

! Doctor found issues in 1 category.

flutter_svg version is 0.22.0

[anglo.svg.zip](https://github.com/dnfield/flutter_svg/files/6463564/anglo.svg.zip)

[dnfield]

You didn't include the SVG itself, but this is almost certainly a duplicate of #102 - this library expects references to be declared before use, which is not exactly what the spec requires but allows for less memory usage/faster parsing.

[zathras]

Yes, I did attach it. It's the markdown thing at the end that refers to https://github.com/dnfield/flutter_svg/files/6463564/anglo.svg.zip - github forced me to put it in a zip file.

Any thoughts as to when the full SVG spec will be implemented?

[dnfield]

As in the readme, this does not intend to conform to the full SVG spec, which would require (among other things) full support for CSS, JavaScript, and foreign object elements.

That said, the smaller ask here of out of order references is trickier than it seems at first. I have no active plans to implement it, but would be willing to review a PR that implemented it efficiently :)

[zathras]

Well, I think the mainline description of this over at pub.dev should mention that it only parses a subset of SVG, in the first sentence. That's much, much too important to bury ten paragraphs down, under a fairly innocuous heading - "Check SVG compatibility" sounds like you're validating the SVG for correctness, and not like you're checking if it's in the subset implemented by this library.

Anyway, the standard technique for dealing with this sort of thing is to put forward reference objects in the tree, and then do a pass over the tree dereferencing them. The technique is well covered in the Dinosaur book. It would be a fair bit of surgery to the code you have now, but a generic DrawableReference<T> where T is the type of drawable would go a fair distance to systematizing things. It takes a small amount of extra memory during parse, and a tiny bit of extra time, but both of those are totally insignificant compared to the other overheads associated with something like SVG. Besides, valuing a slight parsing speedup over correctness is, well, a questionable prioritization.

On a lark, I hacked enough of this to run into some other areas where the incomplete nature of the SVG support convinced me that there's a lot more to do here than just forward references, in order to render typical SVG files (including the ones I have). So I won't be pursuing this, but it helps illustrate the technique. Like I suggested earlier, to do this well you'd want to put it up at the Drawable level, and probably go through a pass over the tree to dereference all the forward references in one go - I won't repeat the whole Dinosaur book here, though :-)

class DrawableStyleableFwd extends DrawableStyleable {
  @override
  final String id;
  final bool nullOk;
  final Map<String, DrawableStyleable> _drawables;
  DrawableStyleable? _referant;

  DrawableStyleableFwd(this.id, this.nullOk, this._drawables) {
    if (nullOk) {
      print("Warning:  DrawableStyleableFwd doesn't handle nullOk");
    }
  }

  DrawableStyleable get referant {
    if (_referant != null) {
      return _referant!;
    }
    final r = _referant = _drawables[id];
    if (r == null) {
      throw StateError('Expected to find Drawable with id $id.\n'
          'Have ids: ${_drawables.keys}');
    } else {
      return r;
    }
  }

  @override
  DrawableStyle? get style => referant.style;

  @override
  Float64List? get transform => referant.transform;

  @override
  DrawableStyleable mergeStyle(DrawableStyle newStyle) {
    if (_referant == null) {
      return DrawableStyleableFwdMerge(this, newStyle);
    } else {
      return (_referant!.mergeStyle(newStyle));
    }
  }

  @override
  void draw(Canvas canvas, Rect bounds) => referant.draw(canvas, bounds);

  @override
  bool get hasDrawableContent => referant.hasDrawableContent;
}

class DrawableStyleableFwdMerge extends DrawableStyleable {
  DrawableStyleableFwd original;
  DrawableStyle newStyle;
  DrawableStyleable? _merged;

  DrawableStyleableFwdMerge(this.original, this.newStyle);

  DrawableStyleable get merged {
    if (_merged != null) {
      return _merged!;
    }
    return _merged = original.referant.mergeStyle(newStyle);
  }

  @override
  String get id => original.id;

  @override
  DrawableStyle? get style => merged.style;

  @override
  Float64List? get transform => merged.transform;

  @override
  DrawableStyleable mergeStyle(DrawableStyle newStyle) =>
      merged.mergeStyle(newStyle);

  @override
  void draw(Canvas canvas, Rect bounds) => merged.draw(canvas, bounds);

  @override
  bool get hasDrawableContent => merged.hasDrawableContent;
}

[dnfield]

Documenting that higher up is completely fair. At one point, the README was significantly shorter and as its grown some of that has gotten spread out further down :)

My largest customers are very sensitive to startup/parsing time and need to run efficiently in relatively low memory environments. My main concern has been that it's better to fail on a feature that would be slow than to silently support it and just be slow. I suppose the library could print out a warning, but this is something that should be pretty easy to preprocess - tools like svgcleaner will fix it for you if you preprocess your SVGs with that.

The problem is both in the parsing and the modeling. Your suggestion wouldn't pass analysis, because it has mutable fields and its base class is @immutable. That can be solved by doing this pass in the parser state logic, but that gets complicated, and there are paths where you end up with several levels of forwarding going on that has to get merged and resolved.

One of my difficulties with this is that it's easily preprocessed away with existing tools.

[zathras]

Your suggestion wouldn't pass analysis, because it has mutable fields and its base class is @immutable

Like I said, the code you're looking at was a quick hack, to see what other obstacles there were. Blowing off @immutable for a ten-minute test hack was entirely intentional. The full solution I outlined would result in an immutable end product.

... there are paths where you end up with several levels of forwarding going on that has to get merged and resolved

Yes. Standard solutions apply here. I don't want to recapitulate the whole compiler construction textbook here (the Dragon book, ISBN 0-321-48681 -- I may have said "Dinosaur" before; it's been a while since undergrad!), but in short, what you do is defer resolution of everything that can't immediately be resolved, then do a depth-first pass over the tree. That way, you in effect start from the leaves, and work your way up - it's a simple recursive algorithm that deals with the multi-level referencing you describe. If your tree is set up right, this part is a line or two of code.

Compared to the overhead of lexing, this kind of semantic tree traversal has a time complexity that is completely insignificant. Perhaps two orders of magnitude faster, typically? In terms of memory usage, it temporarily adds a small amount of memory, of the kind that's extremely efficiently swept away by modern GC implementations.